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

Update migration needed #432

Closed

Conversation

vladshablinsky
Copy link
Contributor

  • 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 tests with your changes locally?

Description

# Formula#migration_needed?

def migration_needed?
  oldname && !rack.exist? && (dir = HOMEBREW_CELLAR/oldname).directory? &&
    !dir.subdirs.empty? && tap == Tab.for_keg(dir.subdirs.first).tap
end

Return false for each of those cases rather than chaining with && since It makes it much easier to follow.

#411 (comment)

cc @MikeMcQuaid

!dir.subdirs.empty? && tap == Tab.for_keg(dir.subdirs.first).tap
return false unless oldname
return false if rack.exist?
return false unless (dir = HOMEBREW_CELLAR/oldname).directory?
Copy link
Member

Choose a reason for hiding this comment

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

Might want to split the dir = out onto the previous line

@MikeMcQuaid
Copy link
Member

👍 other than a few tweaks

@vladshablinsky vladshablinsky force-pushed the tweak_migration_needed branch from 35b6141 to 126d7fa Compare July 1, 2016 15:43
old_cellar_dir = HOMEBREW_CELLAR/oldname

return false unless old_cellar_dir.directory?
return false if old_cellar_dir.subdirs.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a tiny detail, but I think since this and the line above reference old_cellar_dir in their condition, it would be more logical to group them with the old_cellar_dir = line than with the line below, i.e. that's how I would expect the empty lines to be distributed:

return false unless oldname
return false if rack.exist?

old_cellar_dir = HOMEBREW_CELLAR/oldname
return false unless old_cellar_dir.directory?
return false if old_cellar_dir.subdirs.empty?

tap == Tab.for_keg(old_cellar_dir.subdirs.first).tap

Does that make sense?

@UniqMartin
Copy link
Contributor

Two minor comments regarding distribution of empty lines and terminology, but otherwise 👍.

@vladshablinsky vladshablinsky force-pushed the tweak_migration_needed branch from 126d7fa to de576ed Compare July 1, 2016 20:13
@MikeMcQuaid
Copy link
Member

LGTM 👍

@UniqMartin UniqMartin closed this in fbac41d Jul 1, 2016
@UniqMartin
Copy link
Contributor

I agree, hence the merge. The method is now much more readable. 👍

Merged in fbac41d. Thank you for another contribution to Homebrew, @vladshablinsky! 🎉

@vladshablinsky vladshablinsky deleted the tweak_migration_needed branch July 1, 2016 22:38
souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants