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
Include load
in summaries
#216
base: master
Are you sure you want to change the base?
Conversation
a107264
to
aed67db
Compare
Some tests are failing due to differing repository names. I can look into this when there is general consensus on the approach. I'm also happy to make this new behavior configurable in the template. |
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! I updated the branch after the recent churn, which hopefully should fix test failures.
One nit: I would put a blank line between load(...)
and the usage of the symbol, as in buildifier-formatted output.
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.
Instead of hard-coding maybeLoad(...) + summary(...)
in java - expose the load
string to templates, and format it in the template.
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 did that. I got rid of the aspect flag comment as it feels to niche.
Rules, providers, functions and aspects now have a small `load` statement in their summary. Aspects additionally include a copyable `--aspects` flag.
The WORKSPACE tests are failing since they use a different repo name for Stardoc, which now appears in |
Rules, providers, functions and aspects now have a
load
statement in their summary. Aspects additionally include a copyable--aspects
flag.Fixes #95