Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Include load in summaries #216

wants to merge 3 commits into from

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Apr 27, 2024

Rules, providers, functions and aspects now have a load statement in their summary. Aspects additionally include a copyable --aspects flag.

Fixes #95

@fmeum fmeum force-pushed the 95-loads branch 2 times, most recently from a107264 to aed67db Compare April 27, 2024 22:46
@fmeum fmeum marked this pull request as ready for review April 27, 2024 22:52
@fmeum
Copy link
Contributor Author

fmeum commented Apr 27, 2024

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.

@fmeum fmeum changed the title Include load in docs Include load in summaries Apr 27, 2024
Copy link
Collaborator

@tetromino tetromino left a 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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

fmeum added 2 commits May 21, 2024 16:01
Rules, providers, functions and aspects now have a small `load` statement in their summary. Aspects additionally include a copyable `--aspects` flag.
@fmeum
Copy link
Contributor Author

fmeum commented May 21, 2024

The WORKSPACE tests are failing since they use a different repo name for Stardoc, which now appears in load statements. How do you want me to deal with that?

@fmeum fmeum requested a review from tetromino May 22, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show example load() call for each rule
2 participants