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

Added __all__ to many files #1681

Merged
merged 28 commits into from
Apr 21, 2023

Conversation

gran4
Copy link
Contributor

@gran4 gran4 commented Apr 5, 2023

Attempts to fix #1390.
Its unlikely to be perfect yet. I expect a bunch of tests to break

@gran4
Copy link
Contributor Author

gran4 commented Apr 5, 2023

This does it say "arcade.scene.Scene.draw:9:Unexpected indentation". I don't see anything

@einarf
Copy link
Member

einarf commented Apr 5, 2023

This does it say "arcade.scene.Scene.draw:9:Unexpected indentation". I don't see anything

Revert your changes to the docstrings in Scene. You removed a lot of empty newlines. They are there for a reason. Especially the newline between the decription text and the parameter list. There are fairly strict rules on how rst docstrings should be formatted.

Also, your PR is about adding __all__ to many modules. Ideally this should also not contain something random like changing docstrings.

arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
@gran4
Copy link
Contributor Author

gran4 commented Apr 5, 2023

I don't remember touching them. One of my extensions did that or something.

einarf
einarf previously requested changes Apr 6, 2023
arcade/draw_commands.py Outdated Show resolved Hide resolved
arcade/math.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/physics_engines.py Outdated Show resolved Hide resolved
arcade/perf_info.py Outdated Show resolved Hide resolved
arcade/shape_list.py Outdated Show resolved Hide resolved
arcade/sprite/sprite.py Show resolved Hide resolved
arcade/text.py Show resolved Hide resolved
arcade/tilemap/tilemap.py Outdated Show resolved Hide resolved
arcade/version.py Outdated Show resolved Hide resolved
@gran4
Copy link
Contributor Author

gran4 commented Apr 7, 2023

I must have some weird extension. I copy pasted it from arcade bc I messed with it, yet still wanted to commit the file. What should I do? I just deleted it for now.

@einarf
Copy link
Member

einarf commented Apr 7, 2023

I must have some weird extension. I copy pasted it from arcade bc I messed with it, yet still wanted to commit the file. What should I do? I just deleted it for now.

Probably just undo the changes. No need to start from scratch. We squash all the commits anyway so it's not a problem with lots of ugly ones.

@gran4 gran4 requested a review from einarf April 7, 2023 18:29
@einarf
Copy link
Member

einarf commented Apr 8, 2023

Can you just copy scene.py from development branch and replace it with your current one? Add in the __all__ after.

These space changes are so distruptive to git history.

Double check that your own development branch has not diverged from arcade's development branch.

@gran4
Copy link
Contributor Author

gran4 commented Apr 14, 2023

@einarf I'll make a separate pull request

@gran4 gran4 mentioned this pull request Apr 14, 2023
Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

There are still extra indents in scene.py, and there are some additional changes that may need to be made.

The linting step should have picked up those extra spaces, and we should investigate why it isn't doing that as a separate issue (#1701).

arcade/application.py Show resolved Hide resolved
arcade/hitbox/base.py Outdated Show resolved Hide resolved
arcade/physics_engines.py Outdated Show resolved Hide resolved
arcade/scene.py Outdated Show resolved Hide resolved
arcade/scene.py Show resolved Hide resolved
@pushfoo
Copy link
Member

pushfoo commented Apr 20, 2023

You can probably close #1700 since it's redundant now.

Copy link
Collaborator

@cspotcode cspotcode left a comment

Choose a reason for hiding this comment

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

This is a huge improvement, thank you. I think it's suitable to merge.

I did some testing. This does not fix every single __all__ issue, so we should keep #1390 open. I'll share on #1390 how we can use mypy to find the remaining __all__ issues and fix them in a subsequent PR.

Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

I'll follow up on some of the minor lingering style issues after we merge this.

OP has already been through a lot, including helping us discover that our ruff config didn't have whitespace checking enabled (#1701). My guess is their editor or IDE was maintaining previous indent level rather than intelligently managing blank lines. It's an understandable difficulty for a beginner.

@Cleptomania Cleptomania dismissed einarf’s stale review April 21, 2023 21:21

Lingering style issues will be fixed in a second PR

@Cleptomania Cleptomania merged commit 5b6680a into pythonarcade:development Apr 21, 2023
Cleptomania pushed a commit that referenced this pull request Apr 24, 2023
* Make text.py's __all__ consistent with style used in the rest of the codebase

* Fix top of module whitespace in perf_graph.py

* Make pymunk_physics_engine.py consisten with the project's __all__ style

* Tweak spacing
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.

Missing __all__ in some modules prevents mypy --strict from raising useful diagnostics
5 participants