Skip to content

Conversation

@pared
Copy link
Contributor

@pared pared commented Sep 20, 2018

Please take closer look on add_argument, I am not quite sure if i got this right.

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.

Thank you for the PR! Looks very good! A few comments below:

dvc/cli.py Outdated
help='Output DAG as ASCII.')
pipeline_show_parser.add_argument(
'--dot',
dest='filename',
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the dest argument. Without it by default it is going to be dest='dot' which a more suitable name for this.

@efiop
Copy link
Contributor

efiop commented Sep 20, 2018

Ah, one more thing. Tests seem to fail mainly because there is no pydot installed. You would have to specify it in the requirements.txt in order to make tests work.

@pared
Copy link
Contributor Author

pared commented Sep 23, 2018

flake8 fails again and again on line that seems ok to me, my own installation does not show any problems, is there some special config for flake8 on travis?

@efiop
Copy link
Contributor

efiop commented Sep 23, 2018

Looks good! Merged. Thank you!

@efiop efiop merged commit 89e03c8 into treeverse:master Sep 23, 2018
@pared pared deleted the pipeline_show_dot_option branch February 27, 2019 12:21
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