-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set up GitHub Actions CI for JavaScript #7
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the kaitai_compress
is meant to be a library that users will integrate into their applications, the general consensus is that package-lock.json
shouldn't be tracked by Git. See
Actually we should also eliminate yarn.lock
in https://github.com/kaitai-io/kaitai_struct_loader.
…emoved duplicate installation of kaitai-struct-compiler
_test/compile-ksy
Outdated
|
||
ksc -t all --outdir=compiled ksy/*.ksy | ||
for TARGET in javascript python ruby; do | ||
ksc --verbose=all -t $TARGET --outdir=compiled/$TARGET ksy/*.ksy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's better to just specify the -t
parameter multiple times on a single ksc
invocation than launch a separate instance of ksc
for each target, which is unnecessary (and it would also print everything multiple times). Like this:
ksc --verbose all -t javascript -t python -t ruby -d compiled ksy/*.ksy
KSC also adds the compiled/{javascript,python,ruby}/
subdirectories automatically, so you don't need to take care of that anymore.
I'm also not a fan of --verbose all
- this usually fills pages and pages of logs with confused and too detailed stream of unnecessary debug information. My favorite verbosity is --verbose file
, that is usually enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I totally forgot about multiple -t
, let me fix this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. You could also add package-lock.json
to .gitignore
given that we agreed that it doesn't make sense to track it with Git, but that can be done independently of this PR.
ksc \ | ||
--verbose=file \ | ||
-t javascript \ | ||
-t python\ | ||
-t ruby \ | ||
--outdir=compiled \ | ||
ksy/*.ksy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference would be to always use space as a parameter name/value separator, as ksc --help
suggests:
$ ksc --help
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding="UTF-8"
kaitai-struct-compiler 0.10-SNAPSHOT
Usage: kaitai-struct-compiler [options] <file>...
<file>... source files (.ksy)
-t, --target <language> target languages (graphviz, csharp, rust, all, perl, java, go, cpp_stl, php, lua, python, nim, html, ruby, construct, javascript)
-d, --outdir <directory>
output directory (filenames will be auto-generated)
(...)
--verbose <value> verbose output
Note that it's written as --verbose <value>
, not .--verbose=<value>
ksc \ | |
--verbose=file \ | |
-t javascript \ | |
-t python\ | |
-t ruby \ | |
--outdir=compiled \ | |
ksy/*.ksy | |
ksc \ | |
--verbose file \ | |
-t javascript \ | |
-t python\ | |
-t ruby \ | |
--outdir compiled \ | |
ksy/*.ksy |
Sorry for nitpicking.
See kaitai-io/kaitai_compress#7 (comment) Although it doesn't do anything wrong, it's unnecessary.
See kaitai-io/kaitai_compress#7 (comment) Although it doesn't do anything wrong, it's unnecessary.
See kaitai-io/kaitai_compress#7 (comment) Although it doesn't do anything wrong, it's unnecessary.
No description provided.