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

cmd/readall: clean up todos #16011

Merged

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Sep 17, 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?

Edit: We decided to keep the current behavior of running all os/arch combinations and remove the todos and deprecations.

Removes temporary default to readall for every os/arch combination
after updating brew-test-bot and brew/cask to pass the appropriate
arguments in on CI.

Also, add more constants for os/arch combinations. This allows us
to make validating all os/arch combinations the default in
Readall.valid_tap? which is needed to keep the same behavior
in brew tap that we had before. I also updated a few other
spots around the codebase to use those new constants.

One more thing was updating the integration test. In local testing,
this didn't change the runtime so it seemed like a no-brainer.

Todo

  1. The CI used by homebrew/core and homebrew/cask has been updated.
  2. The readall performance improvements PR has been merged in.

@Bo98
Copy link
Member

Bo98 commented Sep 17, 2023

Just to check: does this change the default beahviour of brew readall to be no simulation?

@apainintheneck
Copy link
Contributor Author

Yes, the idea here is to follow-up on the todo to match the behavior described in the original issue and the deprecation notice for the --no-simulate switch. If we want to just keep the default behavior the same as it is now, we could just remove the deprecation notice and tweak things so that all os/arch combinations are passed by default. I haven't really looked into why we chose one default over the other.

@MikeMcQuaid
Copy link
Member

Great work @apainintheneck, loving this so far.

Yes, the idea here is to follow-up on the todo to match the behavior described in the original issue and the deprecation notice for the --no-simulate switch. If we want to just keep the default behavior the same as it is now, we could just remove the deprecation notice and tweak things so that all os/arch combinations are passed by default. I haven't really looked into why we chose one default over the other.

This seems better!

@apainintheneck
Copy link
Contributor Author

@MikeMcQuaid By "This seems better!" do you mean we should continue deprecating the --no-simulate switch or make all os/arch combinations the default instead? I'm not really sure which one you meant.

@apainintheneck
Copy link
Contributor Author

@Bo98 Do you think we should just keep the current behavior as the default and undeprecate (precate?) the --no-simulate switch?

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid By "This seems better!" do you mean we should continue deprecating the --no-simulate switch or make all os/arch combinations the default instead? I'm not really sure which one you meant.

Sorry, the latter but I don't have strong feelings either way!

@apainintheneck
Copy link
Contributor Author

I'm fine with re-enabling the --no-simulate switch in that case. Is there anything special we should do from a user facing perspective for that? I don't think it's really a big deal. Also, I guess this means that the previous two cleanup PRs were unnecessary but I guess it's just clearer to include the options explicitly. We can revert them too but it doesn't really matter either way.

- keep running the command against all os/arch combinations
  as the default
- remove todos and deprecations related to changing the behavior
- create constants for os/arch combinations
@apainintheneck apainintheneck marked this pull request as ready for review September 20, 2023 04:40
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks again @apainintheneck!

@MikeMcQuaid MikeMcQuaid merged commit 1f80e82 into Homebrew:master Sep 20, 2023
24 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 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