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

Support aliases. #116

Merged
merged 1 commit into from
Nov 1, 2015
Merged

Support aliases. #116

merged 1 commit into from
Nov 1, 2015

Conversation

MikeMcQuaid
Copy link
Member

Also some other general cleanup and refactoring and ensuring that tests have 100% coverage.

CC @xu-cheng

@@ -61,6 +56,18 @@ def expand_cask_requirements

private

def self.formulae_info
@@formulae_info ||= begin
Copy link
Member

Choose a reason for hiding this comment

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

I wonder do we need to cache this? Considering, it will only be run once for all of brew bundle subcommands. Beside by removing the cache, we can remove formulae_info_reset! at the same time.

If you want to keep it, you want to change @@formulae_info to @formulae_info

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a class-level cache so it can be reused across instances.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I mean it make more sense to cache the result after it is sorted based on dependency order.

In the core, we prefer to use singleton object variable over class variable. Although both can be used to share data among objects.

Since all of these class are only used once. I wonder should we turn all of them to singleton class? This may help to simplify the code

Copy link
Member Author

Choose a reason for hiding this comment

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

We're using class variables elsewhere so I thought it was better to just be consistent here. Caching after sorted makes sense though 👍

@xu-cheng
Copy link
Member

You need to update Cleanup::formulae_to_uninstall to support alias.

Another problem is that only using name to check formula is not enough. We have to handle the case when full name is used in Brewfile like foo/bar/baz or foo/bar/baz-alias but current installation is from another tap or core.

@MikeMcQuaid
Copy link
Member Author

You need to update Cleanup::formulae_to_uninstall to support alias.

👍

Another problem is that only using name to check formula is not enough. We have to handle the case when full name is used in Brewfile like foo/bar/baz or foo/bar/baz-alias but current installation is from another tap or core.

Will test that case.

@MikeMcQuaid
Copy link
Member Author

It's 💚. @xu-cheng let me know what you think.

@xu-cheng
Copy link
Member

xu-cheng commented Nov 1, 2015

You need to update Cleanup::formulae_to_uninstall to support alias.

You seem to miss that 😉

I don't think we need to handle that case for now unless it's specifically requested.

We still need to somehow support old name. Otherwise, check and cleanup will be broken to users whenever we rename formulae.

@MikeMcQuaid
Copy link
Member Author

You seem to miss that

Whoops, yep, I'll get it.

We still need to somehow support old name. Otherwise, check and cleanup will be broken to users whenever we rename formulae.

That's fine. I don't want to expose old_name as a public API. In those cases users will need to rename. What do you propose we do.

Also some other general cleanup and refactoring and ensuring that tests
have 100% coverage.
MikeMcQuaid added a commit that referenced this pull request Nov 1, 2015
@MikeMcQuaid MikeMcQuaid merged commit daf0bbc into Homebrew:master Nov 1, 2015
@MikeMcQuaid MikeMcQuaid deleted the alias-support branch November 1, 2015 23:40
@lock lock bot added the outdated label Jan 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants