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

feat: detect stardoc output change #75

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

Conversation

alexeagle
Copy link
Contributor

@alexeagle alexeagle commented Oct 2, 2024

Allows Bazel 8 upgrades in rulesets where docgen diff tests otherwise fail, for example https://github.com/aspect-build/rules_lint/pull/401/files

features.bzl Outdated
@@ -33,6 +33,11 @@ _cc = struct(
protobuf_on_allowlist = ge("8.0.0"),
)

_docs = struct(
# See https://bazelbuild.slack.com/archives/CDCMRLS23/p1727889648853179?thread_ts=1727887056.727699&cid=CDCMRLS23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slack links by themselves aren't great in committed code. Could you briefly describe why this is needed and also link the commit that introduced the change?

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 spent a few minutes searching the 300 or so commits since the last rolling release but couldn't find this one. @tetromino do you know offhand?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexeagle - I think you're looking for bazelbuild/bazel@bd1c3af

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Updated.

I see that https://github.com/bazelbuild/stardoc/pull/231/files is the renderer change in Stardoc that corresponds to this. I don't understand this fully - in my testing even the last rolling release of Bazel didn't print the ** on kwargs even though the commit you point out has been included since 8.0.0-pre.20240603.2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me as if the ** would only show up if you combine a Bazel with the linked commit and an up-to-date Stardoc.

@alexeagle alexeagle marked this pull request as ready for review October 3, 2024 14:07
@@ -33,6 +33,12 @@ _cc = struct(
protobuf_on_allowlist = ge("8.0.0"),
)

_docs = struct(
# The stardoc output changed in https://github.com/bazelbuild/bazel/commit/bd1c3af2ea14e81268e940d2b8ba5ad00c3f08d7
# This may be required for "diff tests" that assert on the generated API docs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to make it too difficult to get a new feature in, but I wonder to what extent Stardoc guarantees output stability over time and how that aligns with major/minor releases of Bazel.

Would it make sense to only run diff tests on a single pinned Bazel version so that this kind of feature detection isn't needed? Happy to merge this if you still consider it useful.

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.

3 participants