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.

Sign up for GitHub

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

Jump to bottom

Include load in summaries #216

Merged
merged 9 commits into from
Jun 18, 2024
Merged

Include load in summaries #216

merged 9 commits into from
Jun 18, 2024

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Apr 27, 2024 •
edited
Loading

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 •
edited
Loading

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
tetromino
tetromino requested changes May 20, 2024
View reviewed changes
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.

fmeum added 2 commits May 21, 2024 16:01
@fmeum
Include load in docs
6015ac8 
Rules, providers, functions and aspects now have a small `load` statement in their summary. Aspects additionally include a copyable `--aspects` flag.
@fmeum
Expose loadStatement in template
8e713a0
@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
@tetromino
Copy link
Collaborator

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?

Excellent question! We may have to tag those tests "manual" + bazel version for which the golden was generated; in #232, I am adding a way to auto-update tests whose goldens are valid only for a particular Bazel release.

@tetromino tetromino mentioned this pull request Jun 18, 2024
Merged
@fmeum
Copy link
Contributor Author

fmeum commented Jun 18, 2024

I'll wait for #232 then, let me know what I need to do after it has been merged.

@tetromino
Merge remote-tracking branch 'origin/master' into 95-loads
8f2ff3d
@tetromino
Fork tests and golden files for --noenable_bzlmod mode
3c619d7 
In the interests of brevity and maintainability, we are only interested
in the following tests for --noenable_bzlmod:

* multiple_files
* same_level_file
* table_of_contents
* local_repository (this one needs no forking)
@tetromino
Buildifier
e2101cf
@tetromino
Fix broken merge commit
3ef6018
@tetromino
Give a name to test/bzlmod/MODULE.bazel
afc6cc0 
Otherwise, the test output looks very odd.
tetromino
tetromino approved these changes Jun 18, 2024
View reviewed changes
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.

LGTM.

I took the liberty to fix up the tests, since the merge + necessary tagging and bazelci setup is a bit tricky.

@tetromino tetromino merged commit 018dee5 into bazelbuild:master Jun 18, 2024
18 checks passed
@fmeum fmeum deleted the 95-loads branch June 20, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tetromino tetromino tetromino approved these changes

@brandjon brandjon Awaiting requested review from brandjon brandjon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show example load() call for each rule
2 participants
@fmeum @tetromino