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

Add path expansion for write_junit_xml output argument #5827

Merged
merged 3 commits into from Oct 26, 2018

Conversation

Projects
None yet
3 participants
@EvanKepner
Copy link
Contributor

EvanKepner commented Oct 23, 2018

Hi - this is a small addition to allow the --junit-xml arg to accept a string path as well as a file.

Example:

$ mypy file1.py --junit-xml=a.xml

$ mypy file1.py --junit.xml "this/is/a/test.xml"
$ tree this/
this/
└── is
    └── a
        └── test.xml

2 directories, 1 file
@gvanrossum
Copy link
Member

gvanrossum left a comment

Calling makedirs makes some sense, but user expansion doesn’t.

@EvanKepner

This comment has been minimized.

Copy link
Contributor

EvanKepner commented Oct 24, 2018

Thanks for the review - I included the user expansion to catch cases of ~ in the path e.g. both ~/outputs/file.xml and $HOME/outputs/file.xml are valid path entries where the first is caught by the user expansion, and the latter is caught by the variable expansion. If you'd still like the removal of user expansion let me know and I'll commit the change. If user expansion is removed, then the --junit-xml=~/outputs/file.xml is created in the cwd as /your/cwd/~/outputs/file.xml.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Oct 24, 2018

You can write --junit-xml ~/outputs/file.xml, so there is no need to expand ~/. Please commit the change.

@EvanKepner

This comment has been minimized.

Copy link
Contributor

EvanKepner commented Oct 24, 2018

Thanks for the guidance - in my testing I was passing in the argument as a string with quotes which was my error, when I pass in the argument without the string quotes it operates as expected without the expand-user. Commit made, appreciate the review!

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Oct 24, 2018

I think we probably don't want expandvars either. Shells will do this for us, and we don't do it for any other filenames.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Oct 25, 2018

I think we probably don't want expandvars either. Shells will do this for us, and we don't do it for any other filenames.

Indeed, that's what I meant, sorry.

@EvanKepner

This comment has been minimized.

Copy link
Contributor

EvanKepner commented Oct 25, 2018

No problem at all, changes made.

@gvanrossum gvanrossum merged commit 94c54aa into python:master Oct 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Oct 26, 2018

Thanks!

@EvanKepner

This comment has been minimized.

Copy link
Contributor

EvanKepner commented Oct 26, 2018

Absolutely, thanks for the feedback and accepting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment