Skip to content

Conversation

@rodrigc
Copy link
Contributor

@rodrigc rodrigc commented Jun 30, 2017

Copy link

@opalmer opalmer left a comment

Choose a reason for hiding this comment

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

Looks good 👍

On Windows file system paths are Unicode by default so it's a bit redundant to do .decode(sys.getfilesystemencoding()). The docs for Python 2 mention that getfilesystemencoding() is mainly intended for string -> byte conversion and for Python 3 it apparently changes to always be 'utf-8'. But based on the docs it shouldn't hurt either so I'd leave it as you have it in case there are edge cases out there.

References:
https://docs.python.org/2/library/sys.html#sys.getfilesystemencoding
https://docs.python.org/3/library/sys.html#sys.getfilesystemencoding
https://www.python.org/dev/peps/pep-0529/
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247.aspx

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 30, 2017

@altendky Thanks for #831 , it boosted the coverage of my PR from 95% to 100%

@glyph
Copy link
Member

glyph commented Jul 1, 2017

Thank you @altendky !

Copy link

@opalmer opalmer left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to hit approve on my side.

@glyph
Copy link
Member

glyph commented Jul 1, 2017

What happened to break it again :-( it looks like 95% now.

@rodrigc rodrigc closed this Jul 2, 2017
@rodrigc rodrigc reopened this Jul 2, 2017
@rodrigc
Copy link
Contributor Author

rodrigc commented Jul 3, 2017

If I try to look at https://codecov.io/gh/twisted/twisted/pull/830 , I get a 500 error

@twisted twisted deleted a comment from codecov bot Jul 5, 2017
@rodrigc rodrigc closed this Jul 5, 2017
@rodrigc rodrigc reopened this Jul 5, 2017
@rodrigc
Copy link
Contributor Author

rodrigc commented Jul 5, 2017

I see 100% patch coverage here: https://codecov.io/gh/twisted/twisted/pull/830

@rodrigc rodrigc merged commit 89c53f8 into trunk Jul 5, 2017
@rodrigc rodrigc deleted the 9200-rodrigc-spawnprocess-py3-2 branch July 5, 2017 21:28
@glyph
Copy link
Member

glyph commented Jul 6, 2017

Thanks for investigating @rodrigc.

Hey @codecov - any idea why we keep getting PRs like this where the comment and the build status don't get updated?

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.

3 participants