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

commands: fix completion descriptions #15146

Merged
merged 1 commit into from Apr 4, 2023

Conversation

ZhongRuoyu
Copy link
Member

@ZhongRuoyu ZhongRuoyu commented Apr 4, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The following tests failed but I don't think they are related. (On my Mac, they failed on master, too.)

Failures (output truncated & reformatted)

Failures:
  1. Cask::DSL language stanza when language is set explicitly to 'en' is expected to be the english version
    Failure/Error: it { is_expected.to be_the_english_version }
    ./test/cask/dsl_spec.rb:214:in block (5 levels) in <top (required)>' ./test/support/helper/spec/shared_context/homebrew_cask.rb:52:in block (2 levels) in <top (required)>'

  2. Cask::DSL language stanza when language is set explicitly to 'xx-XX' is expected to be the english version
    Failure/Error: it { is_expected.to be_the_english_version }
    ./test/cask/dsl_spec.rb:220:in block (5 levels) in <top (required)>' ./test/support/helper/spec/shared_context/homebrew_cask.rb:52:in block (2 levels) in <top (required)>'

  3. brew bottle builds a bottle for the given Formula
    Failure/Error:
    expect { brew "bottle", "--no-rebuild", "testball" }
    .to output(/testball--0.1.*.bottle.tar.gz/).to_stdout
    .and not_to_output.to_stderr
    .and be_a_success
    ./test/dev-cmd/bottle_spec.rb:20:in block (2 levels) in <top (required)>' ./test/support/helper/spec/shared_context/integration_test.rb:50:in block (2 levels) in <top (required)>'


Currently, zsh and fish shell completions show incomplete descriptions
for certain commands. For example:

docs                         -- Open Homebrew's online documentation (https://docs
rbenv-sync                   -- Create symlinks for Homebrew's installed Ruby versions in ~/

This is because Commands.command_description produces incomplete
short descriptions for the commands having a dot (from a URL or a path)
in the first sentence; the dot is misinterpreted as a full stop:

brew(main):001:0> Commands.command_description("docs", short: true)
=> "Open Homebrew's online documentation (https://docs"
brew(main):002:0> Commands.command_description("rbenv-sync", short: true)
=> "Create symlinks for Homebrew's installed Ruby versions in ~/" 

We can improve the sentence splitting logic by only splitting at dots
either at the end or followed by a whitespace. Now With this change:

brew(main):001:0> Commands.command_description("docs", short: true)
=> "Open Homebrew's online documentation (https://docs.brew.sh) in a browser"
brew(main):002:0> Commands.command_description("rbenv-sync", short: true)
=> "Create symlinks for Homebrew's installed Ruby versions in ~/.rbenv/versions"

Currently, zsh and fish shell completions show incomplete descriptions
for certain commands. For example:

    docs                         -- Open Homebrew's online documentation (https://docs
    rbenv-sync                   -- Create symlinks for Homebrew's installed Ruby versions in ~/

This is because `Commands.command_description` produces incomplete
short descriptions for the commands having a dot (from a URL or a path)
in the first sentence; the dot is misinterpreted as a full stop:

    brew(main):001:0> Commands.command_description("docs", short: true)
    => "Open Homebrew's online documentation (https://docs"
    brew(main):002:0> Commands.command_description("rbenv-sync", short: true)
    => "Create symlinks for Homebrew's installed Ruby versions in ~/"

We can improve the sentence splitting logic by only splitting at dots
either at the end or followed by a whitespace. Now With this change:

    brew(main):001:0> Commands.command_description("docs", short: true)
    => "Open Homebrew's online documentation (https://docs.brew.sh) in a browser"
    brew(main):002:0> Commands.command_description("rbenv-sync", short: true)
    => "Create symlinks for Homebrew's installed Ruby versions in ~/.rbenv/versions"

Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Nice catch.

@carlocab carlocab merged commit 845506b into Homebrew:master Apr 4, 2023
24 checks passed
@ZhongRuoyu ZhongRuoyu deleted the completions-short-desc branch April 4, 2023 12:39
@@ -203,7 +203,7 @@ def command_description(command, short: false)

if (cmd_parser = Homebrew::CLI::Parser.from_cmd_path(path))
if short
cmd_parser.description.split(".").first
cmd_parser.description.split(/\.(?>\s|$)/).first
Copy link
Member

Choose a reason for hiding this comment

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

@ZhongRuoyu Could this get a Ruby comment added above clarifying what this is doing and why? It's not obvious from reading the code alone. Thanks!

ZhongRuoyu added a commit to ZhongRuoyu/brew that referenced this pull request Apr 4, 2023
A comment here would help the reader to understand the need for this
splitting logic, which is not so straightforward.

Addresses review comment in
Homebrew#15146 (comment).

Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>
ZhongRuoyu added a commit to ZhongRuoyu/brew that referenced this pull request Apr 4, 2023
This was missed in Homebrew#15146.

Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>
ZhongRuoyu added a commit to ZhongRuoyu/brew that referenced this pull request Apr 13, 2023
As I mentioned in Homebrew#15146, two `Cask::DSL` tests failed on my local
machine, even on `master`. `git bisect` suggested that it was Homebrew#14998
that introduced those failures. It turned out that the tests here could
fail under certain locale settings, like this one below:

    $ defaults read -g AppleLanguages
    (
        "en-GB",
        "zh-Hans-SG"
    )

This is not actually a regression. With the aforementioned locale
settings, an explicit `let(:languages) { ["en"] }` setting would result
in locales being considered in the following order: `en`, `en-GB`,
`zh-Hans-SG`. For each of them, the `detect` method from `Locale` is
called, with `locale_groups` as `[["zh"], ["en-US"]]`, the list of
locales defined in the test cask.

    def detect(locale_groups)
      locale_groups.find { |locales| locales.any? { |locale| eql?(locale) } } ||
        locale_groups.find { |locales| locales.any? { |locale| include?(locale) } }
    end

Neither of `en` and `en-GB` satisfies the `detect` conditions. (Note
that `Locale.parse("en").include?("en-US")` evaluates to `false`.) But
`zh-Hans-SG` does (because `Locale.parse("zh-Hans-SG").include?("zh")`
is `true`). So, despite having `:languages` set to `en`, the Chinese
locale was still used.

This could be fixed by generalising the test cask's English locale
settings from `en-US` to `en`. This is already the case for most
existing casks:

    $ grep 'language "en.*", default: true' Casks/*.rb
    Casks/battle-net.rb:  language "en", default: true do
    Casks/cave-story.rb:  language "en", default: true do
    Casks/firefox.rb:  language "en", default: true do
    Casks/libreoffice-language-pack.rb:    language "en-GB", default: true do
    Casks/libreoffice-language-pack.rb:    language "en-GB", default: true do
    Casks/openoffice.rb:  language "en", default: true do
    Casks/seamonkey.rb:  language "en-US", default: true do
    Casks/thunderbird.rb:  language "en", default: true do
    Casks/wondershare-edrawmax.rb:  language "en", default: true do

Note that this should make the language stanza tests independent of
locale settings, because `zh` and `us` should be able to capture all the
test cases.

Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>
ZhongRuoyu added a commit to ZhongRuoyu/brew that referenced this pull request Apr 13, 2023
As I mentioned in Homebrew#15146, two `Cask::DSL` tests failed on my local
machine, even on `master`. `git bisect` suggested that it was Homebrew#14998
that introduced those failures. It turned out that the tests here could
fail under certain locale settings, like this one below:

    $ defaults read -g AppleLanguages
    (
        "en-GB",
        "zh-Hans-SG"
    )

This is not actually a regression. With the aforementioned locale
settings, an explicit `let(:languages) { ["en"] }` setting would result
in locales being considered in the following order: `en`, `en-GB`,
`zh-Hans-SG`. For each of them, the `detect` method from `Locale` is
called, with `locale_groups` as `[["zh"], ["en-US"]]`, the list of
locales defined in the test cask.

    def detect(locale_groups)
      locale_groups.find { |locales| locales.any? { |locale| eql?(locale) } } ||
        locale_groups.find { |locales| locales.any? { |locale| include?(locale) } }
    end

Neither of `en` and `en-GB` satisfies the `detect` conditions. (Note
that `Locale.parse("en").include?("en-US")` evaluates to `false`.) But
`zh-Hans-SG` does (because `Locale.parse("zh-Hans-SG").include?("zh")`
is `true`). So, despite having `:languages` set to `en`, the Chinese
locale was still used.

This could be fixed by generalising the test cask's English locale
settings from `en-US` to `en`. This is already the case for most
existing casks:

    $ grep 'language "en.*", default: true' Casks/*.rb
    Casks/battle-net.rb:  language "en", default: true do
    Casks/cave-story.rb:  language "en", default: true do
    Casks/firefox.rb:  language "en", default: true do
    Casks/libreoffice-language-pack.rb:    language "en-GB", default: true do
    Casks/libreoffice-language-pack.rb:    language "en-GB", default: true do
    Casks/openoffice.rb:  language "en", default: true do
    Casks/seamonkey.rb:  language "en-US", default: true do
    Casks/thunderbird.rb:  language "en", default: true do
    Casks/wondershare-edrawmax.rb:  language "en", default: true do

Note that this should make the language stanza tests independent of
locale settings, because `zh` and `en` should be able to capture all the
test cases.

Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>
@github-actions github-actions bot added the outdated PR was locked due to age label May 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants