-
Notifications
You must be signed in to change notification settings - Fork 129
Make generate_wrappers work on dirty builds #400
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
Conversation
|
@redfire75369 can you check if it works for you. |
|
Yes, it works. Seems fine to me |
09fbf63 to
31e6097
Compare
|
Is it possible to refactor the call to find and the parsing of its output into a sh function in order to avoid so much duplication? |
31e6097 to
f3d42d0
Compare
|
I wish I had this idea before. |
mrobinson
left a comment
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 with one naming nit.
mozjs/src/generate_wrappers.sh
Outdated
| rustfmt target/jsapi.rs --config max_width=1000 | ||
| cp target/debug/build/mozjs-*/out/gluebindings.rs target/glue.rs | ||
| rustfmt target/glue.rs --config max_width=1000 | ||
| find_fmt_parse() { |
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.
How about find_latest_version_of_file_parse_and_format?
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.
That is so loooooong. Maybe just last_file_parse?
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.
Bits are very cheap. It is better to be verbose and obvious, rather than short and obscure. Many people will read this code in the future and it took me a bit to understand it.
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.
used long name and added usage comment and some more comments inside function
f3d42d0 to
2acdf94
Compare
|
Thank you! |
Fixes #398