Skip to content

Conversation

@n3hrox
Copy link

@n3hrox n3hrox commented Nov 10, 2019

  • ❗ Have you followed the guidelines in the Contributing to DVC list?

  • πŸ“– Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

  • ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addresses. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @n3hrox ! πŸ™ Please see some comments up above.

dvc/updater.py Outdated
"formula": "Run {yellow}brew{reset} upgrade dvc",
"apt": (
"rpm": "Run {yellow}yum{reset} update dvc",
"osxpkg": "Run {yellow}brew{reset} upgrade dvc",
Copy link
Contributor

Choose a reason for hiding this comment

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

osxpkg is not the same as brew formula. We need to add build.py with PKG = 'formula' or PKG = "brew"(this one is even better πŸ™‚ ) to our homebrew formula https://github.com/iterative/homebrew-dvc/blob/master/Formula/dvc.rb (after that we'll submit the same changes to homebrew-core).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could also echo somewhere there, need to take a closer look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll include that into our and homebrew-core formulas after this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record: Added iterative/homebrew-dvc@68d0f02 . Will play around with it and then release a new version and submit it to homebrew-core.

dvc/utils/pkg.py Outdated


def is_conda():
def get_package_manager():
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, no good reason to make this dynamic, let's just:

try:
    from .build import PKG  # file created during dvc build
except ImportError:
    PKG = None

and then just import PKG where needed, instead of get_package_manager

Copy link
Author

Choose a reason for hiding this comment

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

DeepSource shows that as anti-pattern, do you still want to keep without function wrap?

Copy link
Contributor

Choose a reason for hiding this comment

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

@n3hrox DS is saying that PKG is unused, which is on purpose here, so we are fine πŸ™‚

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Looks great! πŸ™‚ Just a few minor comments left.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Outstanding job! πŸ₯‡ Thank you so much @n3hrox ! πŸ™

@efiop efiop merged commit 204aa9c into treeverse:master Nov 10, 2019
efiop added a commit to iterative/homebrew-dvc that referenced this pull request Nov 10, 2019
efiop added a commit that referenced this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants