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

imp_url: make default stage fname accompany import target #3312

Merged
merged 1 commit into from Feb 14, 2020

Conversation

pared
Copy link
Contributor

@pared pared commented Feb 12, 2020

  • ❗ 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.
    Done in iterative/dvc.org@c671bed

  • ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. 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. πŸ™


Fixes #3304

@@ -502,12 +502,11 @@ def is_cached(self):
return True

@staticmethod
def create(repo, **kwargs):
def create(repo, accompany_outs=False, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was the one to introduce the kwargs instead of list of arguments, but it seems to me that I overdone that, arguments required in this method should be present in arguments list and if necessary, passed down. kwargs occludes what is necessary for this method.

@pared pared requested review from efiop and skshetry February 12, 2020 11:12
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Looks good. I tried locally, and it worked. :)

@efiop efiop merged commit 5b737a0 into iterative:master Feb 14, 2020
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 14, 2020

Thanks!

Check this box if this PR does not require documentation updates

Were you able to check this doesn't require doc updates? Maybe in the dvc import examples? If you find any you can just open/find a corresponding issue in iterative/dvc.org

UPDATE: Not in examples, but I think the description of -o in https://dvc.org/doc/command-reference/import#options needs a small update. Unchecked box

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 14, 2020

OK never mind, I included this in iterative/dvc.org/pull/993 but please check the docs in future PRs before checking the box. Thanks!

shcheklein pushed a commit to iterative/dvc.org that referenced this pull request Feb 15, 2020
* cmd ref: add space after emoji
intro in https://github.com/iterative/dvc.org/pull/951/files

* term: under X control -> tracked by X (or similar) (1)
for #719

* term: under X control -> tracked by X (or similar) (2)
strings from core repo

* term: add to -> add with (DVC)

* cmd ref: update import -o behaviod explantation
per iterative/dvc/pull/3312
@pared pared deleted the 3304_import_stage branch March 24, 2020 09:36
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.

import: behave like add, move
4 participants