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

Generate fish shell completion at build time #12249

Closed

Conversation

akirabaruah
Copy link
Contributor

@akirabaruah akirabaruah commented Oct 10, 2020

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

@akirabaruah
Copy link
Contributor Author

@uri-canva FYI

@uri-canva
Copy link
Contributor

Have tested it and can build it, and it fixes all related issues:
#12206
#12207
#12208
#12209
#12210

@uri-canva
Copy link
Contributor

@meisterT are you the right person to review it since you reviewed the initial implementation in #11450?

@meisterT meisterT self-requested a review October 13, 2020 07:40
@meisterT
Copy link
Member

@uri-canva yes, I'll review these. Thanks for testing!

Copy link
Member

@meisterT meisterT left a 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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #12267

@akirabaruah akirabaruah force-pushed the fish-completion-gen-static branch from d7501b7 to d76c2f6 Compare October 13, 2020 23:43
@akirabaruah
Copy link
Contributor Author

Please also mention the other issues that this PR fixes.

Done!

return subprocess.check_output(
(self._bazel, '--output_user_root={}'.format(
self._output_user_root)) + tuple(args),
text=True)
Copy link
Member

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?

Copy link
Contributor Author

@akirabaruah akirabaruah Oct 14, 2020

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.

scripts/generate_fish_completion.py Show resolved Hide resolved
scripts/generate_fish_completion.py Show resolved Hide resolved
scripts/generate_fish_completion.py Outdated Show resolved Hide resolved
@akirabaruah akirabaruah force-pushed the fish-completion-gen-static branch 2 times, most recently from 0311392 to 8520e2e Compare October 14, 2020 17:57
akirabaruah added a commit to akirabaruah/bazel that referenced this pull request Oct 14, 2020
Copy link
Member

@meisterT meisterT left a 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

scripts/generate_fish_completion.py Show resolved Hide resolved
scripts/generate_fish_completion.py Show resolved Hide resolved
akirabaruah added a commit to akirabaruah/bazel that referenced this pull request Oct 15, 2020
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
@akirabaruah akirabaruah force-pushed the fish-completion-gen-static branch from 8520e2e to b5ad919 Compare October 15, 2020 18:38
@akirabaruah
Copy link
Contributor Author

two more linter errors and we are good to go

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 :)

@meisterT
Copy link
Member

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

@akirabaruah
Copy link
Contributor Author

Makes sense - thanks! (no rush from my end 😄 )

bazel-io pushed a commit that referenced this pull request Oct 16, 2020
See original request in
#12249 (comment)

Closes #12267.

PiperOrigin-RevId: 337467923
@bazel-io bazel-io closed this in 20febac Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment