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 additional rule info while exporting builtins #21135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

withered-magic
Copy link

@withered-magic withered-magic commented Jan 30, 2024

My WIP Starlark LSP currently uses the ApiExporter to dump all types + globals to the builtins.pb proto, which the LSP then loads at runtime to provide type information/hover docs for Bazel builtins. Right now, the ApiExporter only sets the name and is_mandatory fields of rule Params, limiting the ability of the LSP to provide type checking for rules (e.g. validating argument types). This PR adds additional getters on RuleDocumentationAttribute for the attribute type, default value, documentation, allowing the LSP to use this additional information.

Copy link

google-cla bot commented Jan 30, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jan 30, 2024
@withered-magic withered-magic force-pushed the withered-magic/additional-rule-attr-info branch from 05c174a to 5fd15b2 Compare January 30, 2024 02:50
@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author Starlark configuration Starlark transitions, build_settings team-Documentation Documentation improvements that cannot be directly linked to other team labels and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 30, 2024
@withered-magic
Copy link
Author

withered-magic commented Jan 30, 2024

@sgowroji Any pointers on getting CI to pass? Looks like a potential flake but I can't see the retry button in the usual place in the Buildkite UI.

@keertk
Copy link
Member

keertk commented Feb 2, 2024

Retried and looks good now.

@keertk keertk removed the awaiting-user-response Awaiting a response from the author label Feb 2, 2024
@sgowroji sgowroji added team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols awaiting-review PR is awaiting review from an assigned reviewer labels Feb 2, 2024
@withered-magic
Copy link
Author

Thank you!

@withered-magic withered-magic force-pushed the withered-magic/additional-rule-attr-info branch from 5fd15b2 to 187fce3 Compare April 8, 2024 03:37
@comius comius requested review from tetromino and removed request for brandjon and comius May 15, 2024 13:49
@comius comius added team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob and removed Starlark configuration Starlark transitions, build_settings team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols labels May 15, 2024
@tetromino
Copy link
Contributor

I don't think we should continue trying to represent a native rule class as a callable builtin.Value. Instead, we should simply represent it using a message specifically designed for describing a rule class: a stardoc_output.RuleInfo.

See also discussion in #21929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants