Skip to content

Conversation

@Suor
Copy link
Contributor

@Suor Suor commented Dec 9, 2019

Closes #2492 finally.

@Suor Suor requested review from efiop and pared December 9, 2019 12:59
@efiop efiop requested a review from pared December 11, 2019 18:08
@Suor
Copy link
Contributor Author

Suor commented Dec 12, 2019

@efiop this should be it.

@efiop efiop requested a review from a user December 12, 2019 22:05
@shcheklein
Copy link
Member

@Suor we need to update the command reference doc to explain this option, please

is_write = self.args.unset or self.args.value is not None
if is_write and self.args.name == "cache.type":
logger.warning(
"You have changed cache.type, this doesn't update link types "
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You have changed the `cache.type` option.
This doesn't update existing file links/copies in
the workspace, but it can be done with:

dvc checkout --relink

I'm not sure this formatting with 12 spaces before the command is consistent with other dvc output, like when you dvc add and it gives you the exact git add command, in which case it seems to use a tab\t instead.

Copy link
Contributor Author

@Suor Suor Dec 13, 2019

Choose a reason for hiding this comment

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

Without those spaces it looks really weird on a wider screen:

WARNING: You have changed the 'cache.type' option ...
    dvc checkout --relink

Since we don't have any policy about it, let's keep it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this you used most of my suggestion, thanks. Also this is merged and I don't know if there's an output indentation policy but my 2c here is that the default should be to keep things consistent with existing instances, and I see mainly tabs in existing output (not sure though). Up to you guys cc @efiop

Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgeorpinel Good point. We don't have any set style right now, so I would leave it as is for now and just keep the thought in my head to unify this later. Thanks for the heads up! 🙂

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

👍

@Suor
Copy link
Contributor Author

Suor commented Dec 13, 2019

Made messages go smoother.

@efiop efiop merged commit 8de5286 into iterative:master Dec 13, 2019
@Suor Suor self-assigned this Dec 15, 2019
Comment on lines +36 to +37
" any existing workspace file links, but it can be done with:"
"\n dvc checkout --relink"
Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 17, 2019

Choose a reason for hiding this comment

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

Continuation of #2922 (comment)

Also, I would've put the \n in the previous line.

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.

optimize dir checkout

5 participants