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.

Sign up for GitHub

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

Jump to bottom

cmd/pyenv-sync: add new command. #15507

Merged
merged 2 commits into from
Jun 29, 2023
Merged

cmd/pyenv-sync: add new command. #15507

merged 2 commits into from
Jun 29, 2023

Conversation

johndbritton
Copy link
Member

Similar to rbenv-sync and nodenv-sync, but for use with pyenv.

Python has separate formulae for minor Python versions, as such this will symlink all patch versions to the latest minor version.

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

Following the style in #14972, didn't see any example tests.

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

samford
samford reviewed Jun 5, 2023
View reviewed changes
Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
carlocab
carlocab reviewed Jun 6, 2023
View reviewed changes
Library/Homebrew/cmd/pyenv-sync.rb Outdated
ensure
pyenv_sync_running.unlink if pyenv_sync_running.exist?
Copy link
Member

Choose a reason for hiding this comment

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

Would this not also end up calling unlink after a

   return if pyenv_sync_running.exist?

from above? If so, then that's probably not the desired behaviour here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm following the pattern here:

brew/Library/Homebrew/cmd/rbenv-sync.rb

Lines 46 to 47 in c5f4d04

ensure
rbenv_sync_running.unlink if rbenv_sync_running.exist?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but we probably don't want to follow that since it calls unlink even when not desired. You can avoid this by wrapping the relevant bits in a begin ... end block. See suggestion below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I open a PR to fix this in the other sync commands?

Copy link
Member

Choose a reason for hiding this comment

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

@johndbritton could just do that in this PR I reckon

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough, will update

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed the same issue in rbenv-sync and nodenv-sync in 24c014b.

@github-actions
Copy link

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 28, 2023
@johndbritton
Copy link
Member Author

Took another pass at this, let me know if there are any other concerns.

@github-actions github-actions bot removed the stale No recent activity label Jun 28, 2023
samford
samford reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

Outside of a couple small tweaks (suggestions below), this looks to be in line with the existing nodenv-sync and rbenv-sync commands. I haven't tested the functionality but this approach makes more sense to me 👍

Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
samford
samford reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

The suggestions below should fix the brew style failures.

Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
@johndbritton
Copy link
Member Author

The suggestions below should fix the brew style failures.

I had fixed these, but I guess I forgot to push them, should be fixed now

@MikeMcQuaid MikeMcQuaid requested review from Bo98, samford and carlocab June 29, 2023 09:39
carlocab
carlocab reviewed Jun 29, 2023
View reviewed changes
Library/Homebrew/cmd/pyenv-sync.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/pyenv-sync.rb Outdated
ensure
pyenv_sync_running.unlink if pyenv_sync_running.exist?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, but we probably don't want to follow that since it calls unlink even when not desired. You can avoid this by wrapping the relevant bits in a begin ... end block. See suggestion below.

@johndbritton
cmd/pyenv-sync: add new command.
03b455f 
Similar to `rbenv-sync` and `nodenv-sync`, but for use with `pyenv`.

Python has separate formulae for minor Python versions, as such this
will symlink all patch versions to the latest minor version.
@johndbritton
Copy link
Member Author

Yes, but we probably don't want to follow that since it calls unlink even when not desired. You can avoid this by wrapping the relevant bits in a begin ... end block. See suggestion below.

@carlocab I made the requested changes.

carlocab
carlocab approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks; nice work!

@MikeMcQuaid MikeMcQuaid enabled auto-merge June 29, 2023 17:26
samford
samford approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

Did some quick testing and it seems to work as described, so it looks good to me. Thanks!

@MikeMcQuaid MikeMcQuaid merged commit 95da50f into Homebrew:master Jun 29, 2023
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @johndbritton!

@johndbritton johndbritton deleted the pyenv-sync branch June 30, 2023 14:36
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid left review comments

@carlocab carlocab carlocab approved these changes

@samford samford samford approved these changes

@Bo98 Bo98 Awaiting requested review from Bo98

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@johndbritton @MikeMcQuaid @Bo98 @samford @carlocab