Skip to content
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

Attempts to add docs strings and Type hints #808

Closed
wants to merge 27 commits into from

Conversation

gran4
Copy link
Contributor

@gran4 gran4 commented Apr 17, 2023

No description provided.

@gran4
Copy link
Contributor Author

gran4 commented Apr 17, 2023

Does pyglet not have built in tests?

Something is probably wrong
@gran4
Copy link
Contributor Author

gran4 commented Apr 18, 2023

What is the type returned by open()?

pyglet/event.py Fixed Show fixed Hide fixed
pyglet/event.py Fixed Show fixed Hide fixed
@benmoran56
Copy link
Member

Thanks for working on this. I have a few suggestions, but I'll add them later as in-line comments.

There is an issue, however. Merging this is blocked because automated tests are failing. There is a circular import leading back to pyglet/tests/conftest.py. This file does import several fixtures, including importing the event_loop.

I'm not positive, but the from __future__ import annotations might be the culprit here. Strange that it only happens on OSX and Linux.

@benmoran56
Copy link
Member

I'm confused about the types of _DefaultLoader.

resource.path is just a list of str (directory paths).

@gran4
Copy link
Contributor Author

gran4 commented Apr 20, 2023

Thanks

I'm not positive, but the from __future__ import annotations might be the culprit here. Strange that it only happens on OSX and Linux.

What should I do?

pyglet/event.py Fixed Show fixed Hide fixed
@gran4
Copy link
Contributor Author

gran4 commented Apr 21, 2023

I'm not positive, but the from __future__ import annotations might be the culprit here. Strange that it only happens on OSX and Linux.

I think I broke it more becuase I added from __future__ import annotations to more files.

@benmoran56
Copy link
Member

Thanks

I'm not positive, but the from __future__ import annotations might be the culprit here. Strange that it only happens on OSX and Linux.

What should I do?

Why not make a new branch, and then start a pull request with only one file changed? The automated tests will then run on that, and we can verify if the future import is indeed the cause.

@gran4 gran4 mentioned this pull request Apr 22, 2023
@gran4
Copy link
Contributor Author

gran4 commented Apr 23, 2023

If that's the problem, should I just remove from __future__ import annotations?
I'll make a new PR for different files. This one is getting too large.

pyglet/sprite.py Fixed Show fixed Hide fixed
pyglet/shapes.py Fixed Show fixed Hide fixed
@gran4
Copy link
Contributor Author

gran4 commented Apr 25, 2023

In the arcade python library, they use TYPE_CHECKING bool from typing. I think I'll use that. It won't work if it is the typing library creating the problem. Is it the typing library?

pyglet/clock.py Fixed Show fixed Hide fixed
pyglet/event.py Fixed Show fixed Hide fixed
pyglet/math.py Fixed Show fixed Hide fixed
pyglet/resource.py Fixed Show fixed Hide fixed
@benmoran56
Copy link
Member

I'm afraid that after much consideration, it's not going to be possible to merge such as large pull request.
There are many mistakes that I can find on a quick glance, and it's too time consuming and error prone to review them all.

Please break these changes into smaller individual pull requests.
It's better if you stick to a single module, or even a single class (depending on the number of changes).

@benmoran56 benmoran56 closed this May 24, 2023
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.

None yet

2 participants