-
Notifications
You must be signed in to change notification settings - Fork 402
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
Slight cleanup of cargo_build_script_runner
+ ran rustfmt
#538
Conversation
@illicitonion Hey, if you're around this weekend, I'd love a review. I think this one should be quick. If you don't think this code should be touched then I'm happy to have this PR rejected. I get the sense that I'm touching some sensitive code (but maybe that's not well founded?). |
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, thanks! Left a couple of minor suggestions.
For the future, if you're rustfmt
ing things and also changing them, it can be nice to do those in two separate commits, to make it easier to review what you have changed, vs what rustfmt has changed :)
I get the sense that I'm touching some sensitive code (but maybe that's not well founded?).
I don't think it's particularly sensitive - the only thing to know about it is that it currently can't have dependencies, which is why some of it is how it is (e.g. the command line parsing) :)
@illicitonion Ready for another review 😄 |
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 - one tiny typo caught by CI :) Thanks!
@illicitonion CI is green. Sorry about that 😅 |
I was trying to read through this code and found it a bit hard to follow. This (to me) makes it a bit more readable. There should otherwise be no functional change here.