Skip to content

Not all constants in fcntl are available on macos#6807

Merged
srittau merged 10 commits into
python:masterfrom
sobolevn:patch-64
Jan 7, 2022
Merged

Not all constants in fcntl are available on macos#6807
srittau merged 10 commits into
python:masterfrom
sobolevn:patch-64

Conversation

@sobolevn

@sobolevn sobolevn commented Jan 4, 2022

Copy link
Copy Markdown
Member

Let's try this combination

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood

AlexWaygood commented Jan 4, 2022

Copy link
Copy Markdown
Member

IIRC (please check), if sys.platform == "linux" means (bizarrely) "only present on Ubuntu". I think if you want to say "only present on [grab-bag of miscellaneous linux systems]", you need to say if sys.platform != "win32" and sys.platform != "darwin"

Comment thread stdlib/fcntl.pyi Outdated
@github-actions

This comment has been minimized.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@sobolevn

sobolevn commented Jan 4, 2022

Copy link
Copy Markdown
Member Author

Maybe we should add some extra OS to the test? We can't do that natively, but I can add some docker-container tests 🤔

I will add an issue for that.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

github-actions Bot commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Akuli

Akuli commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

IIRC (please check), if sys.platform == "linux" means (bizarrely) "only present on Ubuntu".

This isn't correct. if sys.platform == "linux" is the correct check for "only present on Ubuntu", and also "present on all linux distros". A check like if sys.platform != "win32" and sys.platform != "darwin" will also be true on other posix systems than linux and macos, such as BSD and Solaris, so it isn't the right choice for linux-specific things.

@AlexWaygood

Copy link
Copy Markdown
Member

IIRC (please check), if sys.platform == "linux" means (bizarrely) "only present on Ubuntu".

This isn't correct. if sys.platform == "linux" is the correct check for "only present on Ubuntu", and also "present on all linux distros". A check like if sys.platform != "win32" and sys.platform != "darwin" will also be true on other posix systems than linux and macos, such as BSD and Solaris, so it isn't the right choice for linux-specific things.

Thanks for the correction!

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't double check in depth, but LGTM.

@srittau srittau merged commit 28a5e6b into python:master Jan 7, 2022
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