Skip to content

Conversation

@skshetry
Copy link
Collaborator

@skshetry skshetry commented Feb 19, 2020

Previous plain gc is now not supported. The same behavior can be trigger by
gc -w or gc --workspace. When --all-{tags, commits, branches} are
specified, it will work as if -w is also specified (same as before).

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

treeverse/dvc.org#1013

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

Related to #2325

@skshetry skshetry self-assigned this Feb 19, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

A general thought, not about this PR, - we should stop automatically making command line options into boolean kwargs, e.g. using set() would be more appropriate here.

@skshetry skshetry force-pushed the fix-gc-defaults branch 3 times, most recently from 6408c82 to 3ff044f Compare February 20, 2020 13:45
Previous plain `gc` is now not supported. The same behavior can be trigger by
`gc -w` or `gc --workspace`. When `--all-{tags, commits, branches}` are
specified, it will work as if `-w` is also specified (same as before).
Comment on lines +10 to +11
class InvalidArgumentError(ValueError, DvcException):
"""Thrown if arguments are invalid."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one! Should we use it in otherplaces like dvc/repo/__init__.py and dvc/repo/reproduce.py too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd opt for it on a different PR. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

@skshetry Sure, let's not forget about it though 🙂

@efiop
Copy link
Contributor

efiop commented Feb 21, 2020

@skshetry Let's not forget about docs, please :)

@skshetry
Copy link
Collaborator Author

@efiop, I have created an issue in treeverse/dvc.org#1013

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

LGTM

@efiop efiop merged commit 5101bf1 into treeverse:master Feb 21, 2020
@skshetry skshetry deleted the fix-gc-defaults branch February 21, 2020 10:44
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.

4 participants