livecheck migration: add brew livecheck developer command

[nandahkrishna]

• 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 tests with your changes locally?

---

This PR integrates livecheck into Homebrew/brew as a developer command. It was almost just a copy-paste of files, but there were some style changes and a couple of minor differences as outlined by @samford. I've also brought over the command completions and placed them in the appropriate files (works on zsh, should also work on bash and fish based on my previous trials).

I tested this locally by also incorporating the changes in #8156 (without which we will get an undefined method strategy error), and it works as intended (I also untapped homebrew/livecheck).

Things to discuss:

1. What changes do we make to the livecheck code?
2. What tests do we write?
3. (Minor) Should livecheck be a common command (for completions)?

I will also be working on the RuboCops/audits in parallel. Looking forward to your reviews!

[nandahkrishna]

Also, I'm unfamiliar with how the man change should be handled (that's why CI seems to be failing). I ran brew man locally and it made some changes to the man files, if I should include those changes as well, let me know.

[maxim-belkin]

I ran brew man locally and it made some changes to the man files, if I should include those changes as well, let me know.

That's exactly what you should do.

[nandahkrishna]

Thanks! Just had a look at the brew license PR as well and figured it out, should've done that before haha.

[maxim-belkin]

I wonder if you could add people who contributed to the files that you add in this PR as co-authors: I think this is the right thing to do with such repo-to-repo migrations. You could do it file-by-file (that'll result in as many commits as there are files -- probably a lot) or once for the single commit.

[nandahkrishna]

👍🏼 Totally agree. Maybe it's fine to add everyone as co-authors to the single commit, I think that keeps things cleaner in the PR. However, if you and the others think the attributions should be more fine-grained, I'll do just that.

[maxim-belkin]

I don't have a preference -- this is just a way to acknowledge people's contributions on GitHub. Let's do it in a single commit for now.

[MikeMcQuaid]

First pass! Just to warn you there will likely be a lot of waves of review on this as the existing code is a bit non-idiomatic. I'd also like to see unit tests for all files except Library/Homebrew/dev-cmd/livecheck.rb (which should have tests like Library/Homebrew/test/dev-cmd/linkage_spec.rb).

[nandahkrishna]

The latest commit includes some refactoring done based on your suggestions @MikeMcQuaid.

I've added a dev-cmd/livecheck_spec.rb test similar to linkage_spec, but I'm not sure how exactly we'd have to proceed with testing livecheck's core logic. There's a lot of network operations involved and actually testing a Formula might not be ideal as upstream can be unreliable.

How do you think we should approach tests for the strategies and livecheck 'core' code?

[samford]

There's a lot of network operations involved and actually testing a Formula might not be ideal as upstream can be unreliable.

Quite a lot of livecheck involves network requests and I agree that this can make testing more difficult.

How do you feel about stubbing network requests, @MikeMcQuaid? It seems like we could use something like the webmock gem to avoid external network requests (which can be slow and unreliable) and to control the response for the sake of testing.

[MikeMcQuaid]

I've given this a quick review but not comprehensive. I've tried to make more general comments where I've seen patterns repeat (that may ease search/replace). Given it's 1000 lines of code I wonder whether it's worth splitting up into separate chunks that can go through and be reviewed in separate PRs more easily? I challenge anyone to do a comprehensive review of 1000 lines of code 😭 😂

Could maybe have separate PRs for:

• the dev-cmd and tests
• the Library/Homebrew/livecheck/livecheck.rb file and tests
• the strategies and tests

Looks good so far, though, I just think there will be a fair bit of bashing to get this code a bit more idiomatic.

[MikeMcQuaid]

I've added a dev-cmd/livecheck_spec.rb test similar to linkage_spec, but I'm not sure how exactly we'd have to proceed with testing livecheck's core logic. There's a lot of network operations involved and actually testing a Formula might not be ideal as upstream can be unreliable.

I think ideally split out the methods based on whether they require a network connection or not. Those that do can have :needs_network tests and be really simple.

How do you think we should approach tests for the strategies and livecheck 'core' code?

For strategies I'd imagine it'd be a matter of being a bit like the versions tests; pass in some expected correct and incorrect URLs and ensure the strategy gets the expected results.

How do you feel about stubbing network requests, @MikeMcQuaid? It seems like we could use something like the webmock gem to avoid external network requests (which can be slow and unreliable) and to control the response for the sake of testing.

We've previously avoided this stubbing because, in my experience, your tests then become tests of the stubbing library more than tests of the actual code. To be clear: I don't expect 100% coverage here and if we can sufficiently extract network logic: that code can have very minimal (and in some cases, no) testing and save the more extensive tests for local logic.

[nandahkrishna]

With the latest push, this PR will only deal with:

• Migrating the livecheck command as a dev-cmd to Homebrew/brew.
• Tests for the dev-cmd.
• Completions for livecheck.

I will be creating PRs for the migration of the livecheck module and the strategies. This PR depends on those two, and cannot be merged until they have been merged. I'll reference them here after I create them – #8254, #8255.

[MikeMcQuaid]

A couple of tiny nits but this is looking really good so far 👏🏻

[nandahkrishna]

The following changes have been made in the latest commit:

• Removed ARGV.inspect, retaining only args. I couldn't find a command that prints the args but pr-upload.rb contains something similar during a dry-run. I decided that it would be okay to just leave the puts args there (it's under the -d and -v switches anyway), unless @MikeMcQuaid feels it isn't necessary.
• Retained only one test (which runs livecheck for a test formula).
• Fixed the if style issue.

[MikeMcQuaid]

Looks good! Happy for this to merge when CI is 💚. Great work @nandahkrishna!

[samford]

This relies on #8254 and #8255 to work, so we probably don't want to merge this PR until after those other two are finished and merged, correct?

[MikeMcQuaid]

Agreed.

[samford]

I created a branch in my local Homebrew/brew repository, merged #8254 and this PR into it (rebasing each on the current main branch beforehand), and did some integration testing to make sure that everything works as expected.

It was necessary to add the require "formula" change that I mentioned earlier but after that everything worked properly. I tested all the command line flags in various combinations as well as the presence/absence of a watchlist file. I did a full run across the homebrew-core formulae and compared it to a previous run using the homebrew-livecheck version of brew livecheck and there weren't any unexpected changes.

After the previously-requested change is integrated, I don't see any notable issues preventing this PR from moving forward after #8254 is wrapped up and merged. This will need to be rebased afterward but that should be it unless we run into issues on CI.

[nandahkrishna]

Incorporating the suggestion and rebasing.

[MikeMcQuaid]

Looks good to me. We probably want to ensure we delete the existing https://github.com/homebrew/homebrew-livecheck command and library code when we merge this and archive that repository. It'd also be worth adding to DEPRECATED_OFFICIAL_TAPS in Library/Homebrew/official_taps.rb in this PR, too!

[nandahkrishna]

It'd also be worth adding to DEPRECATED_OFFICIAL_TAPS in Library/Homebrew/official_taps.rb in this PR, too!

Added this 👍🏼

Will rebase and push after #8544 is merged.

[MikeMcQuaid]

We probably want to ensure we delete the existing https://github.com/homebrew/homebrew-livecheck command and library code when we merge this

@nandahkrishna Could you open a PR for this, too? I'll merge it when this is merged.

[nandahkrishna]

Sure Mike 👍🏼

Edit: PR Homebrew/homebrew-livecheck#1339 has been created to remove files from the Tap.

[MikeMcQuaid]

Thanks again @nandahkrishna!