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

dvc: handle multiline command as list #5194

Merged
merged 1 commit into from
Jan 4, 2021
Merged

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Jan 4, 2021

Implements an approach proposed by @skshetry.

Fixes #4373

iterative/dvc.org#2071

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

Implements an approach proposed by @skshetry.

Fixes iterative#4373
@efiop efiop requested a review from skshetry January 4, 2021 05:56
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.

What were the recent motivations for this? πŸ™‚ Anyway, this is good.

@@ -47,7 +47,7 @@ def warn_if_fish(executable):

def _enforce_cmd_list(cmd):
assert cmd
return cmd if isinstance(cmd, list) else [cmd]
return cmd if isinstance(cmd, list) else cmd.splitlines()
Copy link
Member

Choose a reason for hiding this comment

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

A minor thing: What should we do in the dvc.lock file? Should we keep them in a single format (like either a list or a multiline?)? Just that when the user changes the multiline string to a list or vice versa, it should not affect the lock file.

It might be too much to support that way, but just saying that in principle, it'd be better to have a single format in the lock file, but I think it happens rarely in practice and the list and the multiline string might never be equivalent when user refactors them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skshetry Great point! This PR still uses old schema where we save the cmd as is, but it makes total sense to just enforce the list in the new lock format. I guess we can do it in #5128 ?

@efiop
Copy link
Contributor Author

efiop commented Jan 4, 2021

What were the recent motivations for this? slightly_smiling_face Anyway, this is good.

Just addressing your point that we should handle multiline as a list, so we could close that ticket πŸ™‚

@jorgeorpinel
Copy link
Contributor

Please note that I closed iterative/dvc.org#2721 as stale but I understand that's not something that has been released (per iterative/dvc.org#2721 (comment)) right? thanks

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.

Support multiline command?
3 participants