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

brew CLI commands should be namespaced and implement an interface #16814

Closed
1 task done
dduugg opened this issue Mar 4, 2024 · 9 comments · Fixed by #16998
Closed
1 task done

brew CLI commands should be namespaced and implement an interface #16814

dduugg opened this issue Mar 4, 2024 · 9 comments · Fixed by #16998
Labels
features New features

Comments

@dduugg
Copy link
Sponsor Member

dduugg commented Mar 4, 2024

Verification

Provide a detailed description of the proposed feature

Currently, all brew CLI commands live in the same namespace. As a result, the documentation page for the Homebrew currently contains ~400 class methods. Another consequence is that including a module in any command causes Sorbet to include it in every command, leading to avoidable bugs.

Moving commands to individual namespaces would also allow us to define an interface (currently, no two commands can implement the same methods without potentially clobbering each other), providing clarity to future command authors/maintainers.

What is the motivation for the feature?

☝️

How will the feature be relevant to at least 90% of Homebrew users?

Fewer runtime errors due to missing dependencies (though hopefully this is not something 90% of our users are encountering 😹).

What alternatives to the feature have been considered?

Maintaining the status quo?

@dduugg dduugg added the features New features label Mar 4, 2024
@dduugg dduugg changed the title brew CLI commands should be namespaced, implement interfaces brew CLI commands should be namespaced and implement an interface Mar 4, 2024
@Bo98
Copy link
Member

Bo98 commented Mar 5, 2024

As a result, the documentation page for the Homebrew currently contains ~400 class methods.

For this, we probably should be excluding cmd and dev-cmd entirely anyway. If you are needing to do require "(dev-)cmd/*" even internally then you're doing something very wrong. The code in such files is implicitly file-private - with the only "external" code being how the CLI parser calls Homebrew.send(command), though it should never be called anywhere else.

In theory, you should be able to mark everything in every command file as private_class_method/private.

@MikeMcQuaid
Copy link
Member

For this, we probably should be excluding cmd and dev-cmd entirely anyway.
In theory, you should be able to mark everything in every command file as private_class_method/private.

Agreed 👍🏻


That said: I do agree splitting into more modules would be desirable. We'll just need to maintain backwards compatibility so external commands still work using this module (although we can deprecate/disable/remove that behaviour eventually).

@dduugg
Copy link
Sponsor Member Author

dduugg commented Mar 5, 2024

For this, we probably should be excluding cmd and dev-cmd entirely anyway.
In theory, you should be able to mark everything in every command file as private_class_method/private.

Agreed 👍🏻

Soft disagree – I realize https://rubydoc.brew.sh/ is targeted toward surfacing the Formula API, but could be a better resource for brew contributors as well.

(Commands will need a public run method, but, yes, everything else should be private.)

@Bo98
Copy link
Member

Bo98 commented Mar 5, 2024

but could be a better resource for brew contributors as well.

Though the argument is none of these should be used internally through brew either. Though if we make everything private, that should solve the problem really in any case.

The one exception to the rule is subprocess args calls in build.rb, postinstall.rb and test.rb but those should probably be done differently.

@dduugg
Copy link
Sponsor Member Author

dduugg commented Mar 5, 2024

but could be a better resource for brew contributors as well.

Though the argument is none of these should be used internally through brew either. Though if we make everything private, that should solve the problem really in any case.

The one exception to the rule is subprocess args calls in build.rb, postinstall.rb and test.rb but those should probably be done differently.

I'm attempting to articulate that there are use cases for https://rubydoc.brew.sh/ beyond public API browsing, e.g. browsing brew commands (see "Direct Known Subclasses" here, for one example, which this ticket would unlock). Excluding cmd and dev-cmd from the docs would remove this use case. (Though one could argue there are existing CLI commands, e.g. help and commands for this use case, which is fair enough.)

@MikeMcQuaid
Copy link
Member

Soft disagree – I realize rubydoc.brew.sh is targeted toward surfacing the Formula API, but could be a better resource for brew contributors as well.

I think if we want to do this we should probably figure out how to do so better, perhaps through a different site entirely.

I don't have a problem with better documentation there if these APIs are clearly and consistently labeled as "private".

@apainintheneck
Copy link
Contributor

I'm adding this here as a sort of public reminder. The Homebrew::Untap module logic can be easily moved into the Homebrew::Untap command once this new namespace and interface is implemented. No rush, of course, but it'd be good to handle afterwards.

@MikeMcQuaid
Copy link
Member

Thank for noting here @apainintheneck!

@carlocab
Copy link
Member

carlocab commented Apr 2, 2024

Amazing work @dduugg 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features New features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@MikeMcQuaid @dduugg @Bo98 @carlocab @apainintheneck and others