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

Bash completions: cache names of 'doctor' checks #14507

Merged
merged 2 commits into from Feb 24, 2023

Conversation

maxim-belkin
Copy link
Member

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

I propose to cache names of brew doctor checks the first time the list of the checks is obtained. Also, reuse _brew_doctor function for brew dr (and, hence, remove _brew_dr function which is a duplicate of _brew_doctor).

It seems that some work has been done for Bash completions to be generated "automatically" using brew generate-man-completions. Not sure how this PR is going to affect that effort, so tagging in @Rylan12.

@BrewTestBot
Copy link
Member

Review period will end on 2023-02-07 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 5, 2023
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Yeah, as-is I think brew generate-man-completions will overwrite these changes. You might be able to edit the generation code in Completions to get this to work, but tbh it's been a really long time since I've looked at these so I don't remember the intricacies that well anymore.

@maxim-belkin
Copy link
Member Author

Yeah, as-is I think brew generate-man-completions will overwrite these changes. You might be able to edit the generation code in Completions to get this to work, but tbh it's been a really long time since I've looked at these so I don't remember the intricacies that well anymore.

I suggest to improve Bash completions first (e.g., cache completions) and then, if either one of us gets to generate-man-completions -- fix that one to make it generate similar results. Currently, brew generate-man-completions fails for me with the following message:

$ brew generate-man-completions -vd
Writing markdown to /opt/homebrew/docs/Manpage.md
Error: Broken pipe

@MikeMcQuaid
Copy link
Member

I suggest to improve Bash completions first (e.g., cache completions) and then, if either one of us gets to generate-man-completions -- fix that one to make it generate similar results.

Sorry, we're not going to merge something that's going to get immediately reverted by an automated PR.

@maxim-belkin
Copy link
Member Author

Oh, so changes made to Bash completions are not going to persist unless they're implemented in (some other) files that generate them. This makes me suspect that changes I made in #14467 will be overwritten the next time Bash completions are updated.

@Rylan12
Copy link
Member

Rylan12 commented Feb 6, 2023

Yeah, looks like it was removed in #14513

@maxim-belkin
Copy link
Member Author

maxim-belkin commented Feb 6, 2023

That was fast :/ OK, I'll look for a proper place to implement these changes. The fact that generate-man-completions fails on my machine is not helping but... oh, well!

@maxim-belkin
Copy link
Member Author

Converting to a draft for now as more changes are needed

@maxim-belkin maxim-belkin marked this pull request as draft February 6, 2023 21:37
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 7, 2023
@BrewTestBot
Copy link
Member

Review period ended.

@MikeMcQuaid
Copy link
Member

@maxim-belkin what's the latest here?

@maxim-belkin maxim-belkin marked this pull request as ready for review February 24, 2023 01:37
@maxim-belkin
Copy link
Member Author

@maxim-belkin what's the latest here?

Thanks for the reminder, @MikeMcQuaid. PR is ready for your and @Rylan12's review.

I also fixed syntax of the case command. I can do it in a separate PR -- just let me know.

@MikeMcQuaid
Copy link
Member

Thanks @maxim-belkin! Unfortunately looks like some legit/related test failures here.

@maxim-belkin
Copy link
Member Author

Thanks @maxim-belkin! Unfortunately looks like some legit/related test failures here.

Yep. I fixed one test yesterday but didn't notice the other one a few lines of code down.

@MikeMcQuaid MikeMcQuaid merged commit a72573a into Homebrew:master Feb 24, 2023
@MikeMcQuaid
Copy link
Member

Thanks again @maxim-belkin!

@maxim-belkin maxim-belkin deleted the cache-doctor-checks branch February 24, 2023 16:12
@maxim-belkin
Copy link
Member Author

Thanks for merging it in, Mike!

@github-actions github-actions bot added the outdated PR was locked due to age label Mar 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 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

4 participants