-
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
Expand arg locations #809
Expand arg locations #809
Conversation
…es_rust into expand_arg_locations
Just a quick drive by - could you add a test? |
An easy way to do this could be to generate in a genrule a file which sets a |
Oh... maybe you meant I should make a separate test file. Happy to do so if it matters. |
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.
Oh... maybe you meant I should make a separate test file. Happy to do so if it matters.
I personally think it's better to add a small and focused use use of this functionality to the test
module but I'll defer to @hlopko on this one.
Otherwise looks good to me! 😄
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.
LGTM - thanks!
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.
Thank you so so much for a solid PR. Could you update docs of rule attributes that are affected by this PR to mention that macros are expanded?
It looks to me like the only issues remaining here might be small documentation / test changes, although I think I've resolved all of the comments. I don't feel strongly about any of them, so I'd suggest merging this and then changing any of those things in a follow-up, rather than leaving comments here. That will make it easier to work on a California vs. Munich schedule. |
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.
Thank you so much! Looks great to me!
This fixed #801 for me.