Skip to content

add and migrate constants from ctypes/__init__.pyi to _ctypes.pyi #8643

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

Conversation

junkmd
Copy link
Contributor

@junkmd junkmd commented Aug 29, 2022

I have moved some constants in ctypes/__init__.pyi to _ctypes.pyi as defined in the implementation.

I also added some constants defined in _ctypes.

I will move other stuffs eventually, but I will try to do it little by little.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Great stuff, thank you!

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

Hmm, I'm not sure what's up with the pytype crash. This might be as a result of the circular import :(

I'll try bumping the pytype version in CI to see if that helps, since I think pytype's done some work on improving their name-resolution logic recently.

@junkmd
Copy link
Contributor Author

junkmd commented Aug 29, 2022

A similar situation existed here.

#5350 (comment)

@AlexWaygood
Copy link
Member

I'll try bumping the pytype version in CI to see if that helps, since I think pytype's done some work on improving their name-resolution logic recently.

No, looks like that didn't fix it ☹️ (cc. @rchen152)

As a workaround for now, maybe you could leave RTLD_LOCAL and RTLD_GLOBAL as they are in ctypes/__init__.pyi, and add a TODO comment saying that ideally we would re-export them from _ctypes.pyi?

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Aug 29, 2022

We could also use tests/pytype_exclude_list.txt.

@junkmd
Copy link
Contributor Author

junkmd commented Aug 29, 2022

@Akuli
@AlexWaygood

It is curious that no other modules using circular imports have this error, such as importlib.

I will add workarounds to tests/pytype_exclude_list.txt in the hope that someone will do a fix for pytype.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Aug 29, 2022

Circular imports don't automatically mean that pytype will crash. They only crash pytype when hitting some corner case that wasn't considered before.

@github-actions

This comment has been minimized.

@junkmd
Copy link
Contributor Author

junkmd commented Aug 29, 2022

@Akuli
(cc: @AlexWaygood)

I added all the modules in stdlib/ctypes to the list and still got the error.

According to the error messages, some errors seem to occur in various modules importing ctypes.

Do I have to change the pyi file side currently?

@Akuli
Copy link
Collaborator

Akuli commented Aug 29, 2022

Because _typeshed/__init__.pyi imports ctypes, and because lots of other modules use _typeshed, using the exclude list would mean we need to exclude a lot of things. It isn't a good idea after all.

Instead, we can:

  • Wait for pytype to get fixed
  • Try reverting some of the changes in this PR to make pytype pass (and fix them in a future PR once the pytype bug is fixed)

@junkmd
Copy link
Contributor Author

junkmd commented Aug 29, 2022

@Akuli
Thank you for code correction.

@Akuli Akuli requested a review from AlexWaygood August 29, 2022 14:23
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions
Copy link
Contributor

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

@AlexWaygood AlexWaygood merged commit 1c6eb33 into python:master Aug 29, 2022
@junkmd junkmd deleted the _ctypes_add_and_migrate_stuffs_from_public_ctypes_01 branch August 29, 2022 14:52
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