Skip to content

livecheck: raise error if no watchlist#10054

Merged
samford merged 4 commits intoHomebrew:masterfrom
hyuraku:livecheck-raise-error
Dec 20, 2020
Merged

livecheck: raise error if no watchlist#10054
samford merged 4 commits intoHomebrew:masterfrom
hyuraku:livecheck-raise-error

Conversation

@hyuraku
Copy link
Copy Markdown
Contributor

@hyuraku hyuraku commented Dec 18, 2020

  • 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?
  • Have you successfully run brew man locally and committed any changes?

closes #10052
raise an error when brew livecheck has no args and set no ENV['HOMEBREW_LIVECHECK_WATCHLIST'] or ~/.brew_livecheck_watchlist

@MikeMcQuaid MikeMcQuaid requested review from maxim-belkin, nandahkrishna and samford and removed request for samford December 18, 2020 15:12
@samford samford force-pushed the livecheck-raise-error branch 2 times, most recently from 91b9753 to 3bbb4ba Compare December 18, 2020 16:07
@samford
Copy link
Copy Markdown
Member

samford commented Dec 18, 2020

This issue was introduced in #8578 (adding cask support). Before that point, when a user ran brew livecheck without HOMEBREW_LIVECHECK_WATCHLIST set or a ~/.brew_livecheck_watchlist file existing, they would see the following:

$ brew livecheck
Usage: brew livecheck [formulae]

Check for newer versions of formulae from upstream.

If no formula argument is passed, the list of formulae to check is taken from
HOMEBREW_LIVECHECK_WATCHLIST or ~/.brew_livecheck_watchlist.

      --full-name                  Print formulae with fully-qualified names.
      --tap                        Check formulae within the given tap,
                                   specified as user/repo.
      --all                        Check all available formulae.
      --installed                  Check formulae that are currently
                                   installed.
      --newer-only                 Show the latest version only if it's newer
                                   than the formula.
      --json                       Output information in JSON format.
  -q, --quiet                      Suppress warnings, don't print a progress
                                   bar for JSON output.
  -d, --debug                      Display any debugging information.
  -v, --verbose                    Make some output more verbose.
  -h, --help                       Show this message.

Error: Invalid usage: No formulae to check.

I pushed a commit that reinstates this behavior by using the safe navigation operator before sort_by (i.e., end&.sort_by). This caused brew style to want the elsif/else to be in line with the if, so I simply moved the if onto the next line and indented everything. [The alternative was to do the sort separately from the assignment but doing formulae_and_casks_to_check&.sort_by! results in an error about not being able to modify a frozen array.]

That said, the question is whether the existing Error: Invalid usage: No formulae to check. message is sufficient or would we benefit from the more specific UsageError in this PR. On the one hand, the usage banner already contains information about the watchlist. On the other hand, users may overlook that part of the usage banner, so making it clear that the error is related to a missing watchlist file may be helpful to new users.

Maybe we could split the difference and make the watchlist UsageError in this PR a bit more brief and rely on the usage banner for the specific information. Do we think something like A watchlist file is required when no arguments are given. would be sufficient?

@samford samford force-pushed the livecheck-raise-error branch from 3bbb4ba to 7677013 Compare December 18, 2020 16:32
Copy link
Copy Markdown
Member

@nandahkrishna nandahkrishna left a comment

Choose a reason for hiding this comment

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

A watchlist file is required when no arguments are given.

I prefer this because it's possible that the user would miss the information present in the usage banner, and No formulae to check doesn't seem specific enough.

Copy link
Copy Markdown
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

I agree that there's value in having a better error than No formulae or casks to check. in this context. I imagine we're in agreement on this point, so it's just a matter of figuring out the best error message.

I think the suggestion below is a happy medium and users can simply refer to the usage banner if they're not familiar with watchlists.

Comment thread Library/Homebrew/dev-cmd/livecheck.rb Outdated
@samford samford force-pushed the livecheck-raise-error branch from f46bb67 to c7080ba Compare December 20, 2020 04:02
@samford
Copy link
Copy Markdown
Member

samford commented Dec 20, 2020

I'm going to go ahead and merge this since it's fixing a bug. Thanks, @hyuraku!

@samford samford merged commit 2ddb71a into Homebrew:master Dec 20, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 20, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 20, 2021
@hyuraku hyuraku deleted the livecheck-raise-error branch January 27, 2021 12:32
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.

Error: undefined method sort_by for nil:NilClass when running brew livecheck

4 participants