The default behaviour is too strict for unofficial taps. We can still
warn elsewhere but, given the potential for false positives, let's
loosen things a bit.
This upgrades `utils/curl.rb` to `typed: strict`, which requires
a number of changes to pass `brew typecheck`. The most
straightforward are adding type signatures to methods, adding type
annotations (e.g., `T.let`) to variables that need them, and ensuring
that methods always use the expected return type.
I had to refactor areas where we call a `Utils::Curl` method and use
array destructuring on a `SystemCommand::Result` return value
(e.g., `output, errors, status = curl_output(...)`), as Sorbet
doesn't understand implicit array conversion. As suggested by Markus,
I've switched these areas to use `#stdout`, `#stderr`, and `#status`.
This requires the use of an intermediate variable (`result`) in some
cases but this was a fairly straightforward substitution.
I also had to refactor how `Cask::URL::BlockDSL::PageWithURL` works.
It currently uses `page.extend PageWithURL` to add a `url` attribute
but this reworks it to subclass `SimpleDelegator` and use an
`initialize` method instead. This achieves the same goal but in a way
that Sorbet can understand.
Our autobump workflow sets the author and committer to the user who
triggered the workflow, defaulting to @BrewTestBot for scheduled runs.
This can be confusing for maintainers when GitHub shows up as
"Unverified" because the commit is signed with @BrewTestBot's key.[^1]
Let's fix that by configuring our autobump workflow to always commit as
@BrewTestBot, so that the committer matches the GPG signature. To do
that, we need to add support for setting `GIT_COMMITTER_NAME` and
`GIT_COMMITTER_EMAIL`.
[^1]: See, for example, Homebrew/homebrew-core#197234.
`brew bump-formula-pr` is encountering a type error, as the inferred
return type of `GitHub#check_for_duplicate_pull_requests` doesn't
align with the explicit return type of `#check_pull_requests`:
```
Error: Return value: Expected type T.nilable(T::Array[String]), got
type Module with value T::Private::Types::Void::VOID
Caller: /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/dev-cmd/
bump-formula-pr.rb:137
Definition: /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/
dev-cmd/bump-formula-pr.rb:472 (Homebrew::DevCmd::BumpFormulaPr
#check_pull_requests)
```
This addresses the issue by adding a type signature with a `void`
return type to `#check_for_duplicate_pull_requests` and setting the
return type of `#check_pull_requests` to `void` as well. The return
type from `#check_pull_requests` isn't used, so a `void` return type
is arguably a better reflection of the method's behavior. The
`#check_pull_requests` method in `BumpCaskPr` has a `void` return
type, so this change brings the `BumpFormulaPr` method in line.
- change the messaging depending on how confident we are that we're
actually looking at duplicates i.e. we're not confident without a
version number supplied
- similarly, just warn instead of failing with an error (and no
override) if we're not confident that we're looking at duplicates
because a version wasn't supplied
- change `bump-cask-pr` and `bump-formula-pr` to always check for all
pull requests with the new version number (to allow failing on this)
rather than only checking closed pull requests with a version number
- change `bump` to check for definite/maybe duplicate PRs and only
exit if they are definitely duplicates
- cleanup some variable usage to DRY things up a bit
This doesn't count PRs from forks, so it only works correctly for
maintainers (and only if they use non-fork branches).
Let's fix that, and:
- simplify the logic by using `paginate_graphql`
- return early if it's impossible for them to have too many open PRs
- Previously I thought that comments were fine to discourage people from
wasting their time trying to bump things that used `undef` that Sorbet
didn't support. But RuboCop is better at this since it'll complain if
the comments are unnecessary.
- Suggested in https://github.com/Homebrew/brew/pull/18018#issuecomment-2283369501.
- I've gone for a mixture of `rubocop:disable` for the files that can't
be `typed: strict` (use of undef, required before everything else, etc)
and `rubocop:todo` for everything else that should be tried to make
strictly typed. There's no functional difference between the two as
`rubocop:todo` is `rubocop:disable` with a different name.
- And I entirely disabled the cop for the docs/ directory since
`typed: strict` isn't going to gain us anything for some Markdown
linting config files.
- This means that now it's easier to track what needs to be done rather
than relying on checklists of files in our big Sorbet issue:
```shell
$ git grep 'typed: true # rubocop:todo Sorbet/StrictSigil' | wc -l
268
```
- And this is confirmed working for new files:
```shell
$ git status
On branch use-rubocop-for-sorbet-strict-sigils
Untracked files:
(use "git add <file>..." to include in what will be committed)
Library/Homebrew/bad.rb
Library/Homebrew/good.rb
nothing added to commit but untracked files present (use "git add" to track)
$ brew style
Offenses:
bad.rb:1:1: C: Sorbet/StrictSigil: Sorbet sigil should be at least strict got true.
^^^^^^^^^^^^^
1340 files inspected, 1 offense detected
```
Per feedback to https://github.com/Homebrew/brew/pull/17806, this
moves some `require` statements in `dev-cmd/contributions.rb` and
`Utils::GitHub` into the methods that need them.
This resolves `unitialized constant` errors in `brew contributions`
(`Tap`, `GitHub`) and `Utils::GitHub` (`Utils::Curl`).
This also preemptively adds some requires to `Utils::GitHub` and
`GitHub::API`, to avoid similar errors.
- Both `from` and `to` are now separate keyword arguments
in a bunch of places, not part of `args`.
- When we switched this around, we didn't realize this
method needed updating to correctly construct the time
range query.
- This led to further inaccurate counts in `brew contributions`
for reviews, since `from` and `to` are not valid search qualifiers
for the GitHub PR search APIs.
- This was broken (I did have a commit SHA for the breakage but I can't find it now) since `from` and `args.from` are different variables (one can be nil, the other has a default value).
- So it was reporting very high counts because, despite the message, the `from` restriction was not being passed to `count_repo_commits`.
Don't let users open more than 15 PRs at a time. We have other tooling
to nudge them to not do this but let's put it in the worst offenders:
the `bump*` commands.
We early return here
92a4311868/Library/Homebrew/utils/github/api.rb (L220)
, but don't then handle that through the stack.
Repro:
```console
❯ HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_GITHUB_API=1 brew bump-formula-pr --write-only --version 1.2.3 --no-audit jq
Error: undefined method `[]' for nil:NilClass
Do not report this issue until you've run `brew update` and tried again.
Warning: Removed Sorbet lines from backtrace!
/opt/homebrew/Library/Homebrew/utils/github.rb:565:in `block in fetch_pull_requests'
/opt/homebrew/Library/Homebrew/utils/github/api.rb:334:in `paginate_graphql'
/opt/homebrew/Library/Homebrew/utils/github.rb:564:in `fetch_pull_requests'
/opt/homebrew/Library/Homebrew/utils/github.rb:628:in `check_for_duplicate_pull_requests'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:456:in `check_open_pull_requests'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:135:in `run'
/opt/homebrew/Library/Homebrew/brew.rb:89:in `<main>'
Rerun with `--verbose` to see the original backtrace
```
- Some repositories occasionally change their licenses. For example they
release a version of the software with one license and then decide to change
the license later.
- Now that `?ref=` is a parameter to the GitHub Repositories License API,
we can use that in the license audit to check if the license of the specific
release matches the one declared in the formula.
If there are duplicate PRs: we shouldn't suggest and allow a trivial
override. Instead, they should be created manually.
An undocumented override exists for BrewTestBot to do autobumps.
