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

Also skip filtering null modifications when --quiet is passed to brew update #13255

Closed

Conversation

boblail
Copy link
Contributor

@boblail boblail commented May 6, 2022

(We're already skipping this when --preinstall is passed)

This change is orthogonal to (not instead of) optimizing this filtering logic (See #13244, #13243 discussion)


  • 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?

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.

Some optional ideas to tweak naming.

This approach makes sense to me but, given most users will literally only ever use brew update --preinstall, I debate whether we should consider consolidating the behaviour/output because I'm not terribly sure "more output on --preinstall because it's faster" is a very sensible default (despite by deciding it long ago).

Library/Homebrew/cmd/update-report.rb Outdated Show resolved Hide resolved
@boblail
Copy link
Contributor Author

boblail commented May 6, 2022

I'm not terribly sure "more output on --preinstall because it's faster" is a very sensible default

Agreed!! Do you think --preinstall should imply --quiet? If so, I'll make that PR. If not, maybe we can stop altering the behavior for --preinstall after we've found the right optimization.

… to `brew update`

This step is expensive and the results are discarded if the user isn't printing the update report.
@boblail boblail force-pushed the lail/brew-update-no-filter-when---quiet branch from 40b5cd5 to ccccf21 Compare May 6, 2022 15:00
@MikeMcQuaid
Copy link
Member

Agreed!! Do you think --preinstall should imply --quiet? If so, I'll make that PR. If not, maybe we can stop altering the behavior for --preinstall after we've found the right optimization

I'm unsure! Will ask questions in other PR too. Note: I'm on vacation next week so will be unresponsive.

@MikeMcQuaid
Copy link
Member

I think #13299 may remove the need for this but interested in thoughts.

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jun 9, 2022
@boblail
Copy link
Contributor Author

boblail commented Jun 13, 2022

Closing in favor of recent brew update changes.

@boblail boblail closed this Jun 13, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2022
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 stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants