-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Generate fish shell completion at build time #12249
Generate fish shell completion at build time #12249
Conversation
b90f560
to
d7501b7
Compare
@uri-canva FYI |
@uri-canva yes, I'll review these. Thanks for testing! |
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.
Please also mention the other issues that this PR fixes.
@@ -1,4 +1,5 @@ | |||
# fish completions | |||
|
|||
To enable bazel completions, copy //scripts/fish/completions/bazel.fish to | |||
~/.config/fish/completions/bazel.fish. | |||
To enable bazel completions, run `bazel build //scripts:fish_completion` and |
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.
In a separate PR, it would be useful to add that info to https://github.com/bazelbuild/bazel/blob/master/scripts/packages/template_bin.sh#L181 as well.
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.
See #12267
d7501b7
to
d76c2f6
Compare
Done! |
scripts/generate_fish_completion.py
Outdated
return subprocess.check_output( | ||
(self._bazel, '--output_user_root={}'.format( | ||
self._output_user_root)) + tuple(args), | ||
text=True) |
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.
Can you avoid using text
here to keep compatibility with older python versions?
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.
Changed it to universal_newlines
for backwards compatibility. If we outright remove it, it seems like the output will be parsed as binary.
0311392
to
8520e2e
Compare
See original request in bazelbuild#12249 (comment)
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.
two more linter errors and we are good to go
See original request in bazelbuild#12249 (comment)
Fish shell completion for bazel previously relied on parsing bazel help text at run time, leading to latency of multiple seconds for first-time completion. This change adds a script that instead parses bazel help text and generates an appropriate fish completion script at build time, greatly reducing completion latency for the user. Fixes bazelbuild#12206 Fixes bazelbuild#12207 Fixes bazelbuild#12208 Fixes bazelbuild#12209 Fixes bazelbuild#12210
8520e2e
to
b5ad919
Compare
For future reference, which linter commands are you running? I figure it might help to run them locally before uploading my changes to save time :) |
Thanks for your patience. It's a google internal version of pylint, I believe you can replicate the exact behavior by following this instructions: https://google.github.io/styleguide/pyguide.html#21-lint |
Makes sense - thanks! (no rush from my end 😄 ) |
See original request in #12249 (comment) Closes #12267. PiperOrigin-RevId: 337467923
Fish shell completion for bazel previously relied on parsing bazel help
text at run time, leading to latency of multiple seconds for first-time
completion. This change adds a script that instead parses bazel help
text and generates an appropriate fish completion script at build time,
greatly reducing completion latency for the user.
Fixes #12206
Fixes #12207
Fixes #12208
Fixes #12209
Fixes #12210