Skip to content

Conversation

@ymyzk
Copy link
Contributor

@ymyzk ymyzk commented Jun 27, 2018

Use of sys.platform is now recommended for platform-dependent APIs #2275. So, I update stubs which contains # Windows only or # Unix only to use if sys.platform ....

@ymyzk
Copy link
Contributor Author

ymyzk commented Jun 28, 2018

To fix failing tests https://travis-ci.org/python/typeshed/jobs/397420897, should we update tests in mypy, or should we avoid using sys.platform for some APIs to fix failing tests?

@gvanrossum
Copy link
Member

Hm, I see the problem. Many of the O_* flags are present on some platforms and not others, with a finer granularity than sys.platform can indicate. So there's a convention to check for presence of a specific flag, which you see in the failing example code. I think for the O_ flags in particular it would be better to restore the original code (maybe add a clarifying comment as to why it's going against the recommendation).

@ymyzk ymyzk force-pushed the use-sys-platform branch from 805bfe8 to 2a8d3a1 Compare June 28, 2018 15:08
@ymyzk
Copy link
Contributor Author

ymyzk commented Jun 28, 2018

I see the problem. I reverted stubs for O_* flags and add comments why we decided not to use sys.platform for now.

@ymyzk ymyzk force-pushed the use-sys-platform branch from 2a8d3a1 to 6908e5c Compare July 3, 2018 00:31
@JelleZijlstra
Copy link
Member

I wonder if the same problem will happen with other flags changed in this PR. @gvanrossum perhaps you could check this PR against Dropbox's codebase to see if it introduces new mypy errors? (Our typed codebase doesn't tend to use low-level os features.)

@gvanrossum
Copy link
Member

OK checking now...

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

OK, it all works fine with the internal code bases.

@JelleZijlstra
Copy link
Member

Thanks for checking!

@JelleZijlstra JelleZijlstra merged commit 1ae2ba0 into python:master Jul 3, 2018
@ymyzk ymyzk deleted the use-sys-platform branch July 3, 2018 15:17
@gvanrossum
Copy link
Member

PS. In the future don't amend your commits during code review -- just push new commits, it's easier for the reviewers. They'll be squashed when we merge.

@ymyzk
Copy link
Contributor Author

ymyzk commented Jul 3, 2018

Sorry about that. I'll take care next time!

yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
@tungol tungol mentioned this pull request Nov 20, 2024
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