Fixed typos and made adjustments for PEP8 in Python listings #119

Doublestuf · 2024-07-14T05:49:18Z

Description

I fixed some typos, changed variable and function names to match PEP8, and added type hints in Python. Got through about 9.5 out of 14 folders in the Python listing, so about 2/3rds done. Didn't realize until after starting that I cloned the master branch and some type hints were being added in develop, but I didn't reach computer science, collision detection, algorithms, or AI so I don't think we changed any of the same files. More changes could be made for code cleanliness in function and variable names, but I decided not to focus on that for now, can always go through the files again later to do another sweep. I did do some slight variable name changes for code cleanliness/readability (i.e player.on_ground -> player.is_on_ground), but mostly making Python-specific adjustments.

Speaking of which, I noticed that the abc module could be used for implementing abstract classes in quite a few design patterns. I didn't add that because it could raise concerns about the code being understandable for beginners, but it may be worth considering since perhaps the language-specific versions should assume intermediate-or-so level knowledge in that language, not sure. A change I made in match3_fallingtiles.py may not be functionally correct, the function changed would probably benefit from a rewrite for Python due to the differences from how for loops work in C++, although maybe I should be using the Psuedocode version as a reference instead. Related, made a minor change in timer.cpp for more concise code since I was using it as a reference for type hinting, not focusing on C++ as of now though. When adding type hints for vectors, I chose to use Vector2 instead of Vector2D like in the C++ listings because that seems more reasonable based on its usage, and the Python snippets are based on Pygame so it may help for familiarity. From what I can tell, Vector2 is more common in game engines than Vector2D anyways.

I planned on going through the rest of the folders and doing a few more goes through the Python snippets to ideally make them fairly idiomatic Python code before noticing you got started on them, decided to submit what I've got now and see until further notice. My changes could benefit from future revisions, but it's a start I suppose.

Context and motivation

I saw theopen issue for Python type hints (#69) and recognized that I could help (5+ years experience in Python). When completed and revised, my changes will hopefully make the book more helpful for Python users, considering they'd be closer to functioning, usable Python code. Additionally, some changes I made help follow the code cleanliness rules for PEP8, which can help setup good habits for those learning and require less refactoring time for experienced developers.

Type of changes:

Added new chapters
Fixed mistakes
Filled placeholders

Checklist

My contribution follows the contribution guidelines
I added myself in the contributors section (or I am already present)
I consent that my contribution will be distributed under the CC BY-NC-SA License

…PEP8, and added type hints. About 2/3rds done, will need future revisions.

added microStudio to game engine list

Penaz91

Thank you for your contribution, hope you don't mind me asking a few more things to fix! (I'll add more context to this review in the next comment)

dynamic_listings/python/default/containers/timer.py

dynamic_listings/python/default/developing_mechanics/match3_fallingtiles.py

dynamic_listings/python/default/containers/particle_emitter_complex.py

Penaz91 · 2024-07-14T20:10:29Z

Hello! First of all, thanks for taking the time to help this project. I was thinking about making the Python edition more "Pythonic" (by applying PEP8 more strictly and adding Type hints) so you are saving me a ton of work!

I fixed some typos, changed variable and function names to match PEP8, and added type hints in Python. Got through about 9.5 out of 14 folders in the Python listing, so about 2/3rds done. Didn't realize until after starting that I cloned the master branch and some type hints were being added in develop, but I didn't reach computer science, collision detection, algorithms, or AI so I don't think we changed any of the same files. More changes could be made for code cleanliness in function and variable names, but I decided not to focus on that for now, can always go through the files again later to do another sweep. I did do some slight variable name changes for code cleanliness/readability (i.e player.on_ground -> player.is_on_ground), but mostly making Python-specific adjustments.

That is great! Feel free to pull from the develop branch so that you can get my edits too, saving you a bit of effort when using type hints.

Speaking of which, I noticed that the abc module could be used for implementing abstract classes in quite a few design patterns. I didn't add that because it could raise concerns about the code being understandable for beginners, but it may be worth considering since perhaps the language-specific versions should assume intermediate-or-so level knowledge in that language, not sure.

ABCs are a bit of a problematic area: they are more of a way to register a certain class as one that abides by a certain "informal protocol" (like "Hashable", which means it contains the __hash__ method, or "Sized", which means it contains the __len__ method.) so it may not really be the correct way to declare an "Abstract Class" in the style of C++.

A change I made in match3_fallingtiles.py may not be functionally correct, the function changed would probably benefit from a rewrite for Python due to the differences from how for loops work in C++, although maybe I should be using the Psuedocode version as a reference instead

You are probably right, the entire function may benefit from a full rewrite. I'll be cross-checking the editions to see what I can do about it.

I chose to use Vector2 instead of Vector2D like in the C++ listings because that seems more reasonable based on its usage, and the Python snippets are based on Pygame so it may help for familiarity. From what I can tell, Vector2 is more common in game engines than Vector2D anyways.

Absolutely fair, I took inspiration from PyGame myself and maybe, in a far away future, there might be a PyGame edition too (although It's not really a priority, since the "Pygame 4000 book" exists and its cost finances part of the PyGame development itself). I say it's a good idea to switch to Vector2.

I planned on going through the rest of the folders and doing a few more goes through the Python snippets to ideally make them fairly idiomatic Python code before noticing you got started on them, decided to submit what I've got now and see until further notice. My changes could benefit from future revisions, but it's a start I suppose.

This is more than just "a start", if you're okay with working more on this pull request, I'll do my best to assist you every step of the way, until this is merged.

Thank you again for taking the time to contribute!

…and half of collision detection

Doublestuf · 2024-07-15T06:01:42Z

Working on it 👍. The last few folders have a lot more files, took notes as I made changes and ended up with 4 pages for today, will do a write-up soon enough. Glad to have this as my first open-source contribution.

Penaz91

Great job so far! (Don't mind my nitpicking)

To me it seems that you're doing far more than just "Improving Python listings" since you've been giving a glance-over to other languages too. Feel free to write "Improved code listings" inside CONTRIBUTORS.md.

Again, thank you for your contribution!

dynamic_listings/C++/default/containers/fsm_usage.cpp

dynamic_listings/python/default/collisiondetection/AABB.py

Penaz91 · 2024-07-15T12:17:00Z

dynamic_listings/python/default/collisiondetection/brute_force.py

 colliding_items: list[tuple[Circle]] = []

 for A in items:
    for B in items:
-        if A is not B:
+        if A != B:


I'm not really sure about this edit. If the class Circle had an __eq__ operator that uses center and radius to compare two circles, two different circles with the same radius and center would show as "not colliding"; while the is operator will skip the check if the two pointers (A and B) point to the same object (thus two identical circles would collide, as long as they are two different objects).

Penaz91 · 2024-07-15T12:18:04Z

dynamic_listings/python/default/collisiondetection/circle_circle.py

+        self.center = center
+        self.radius = radius


Missing/Removed Type hints

Penaz91 · 2024-07-15T12:18:33Z

dynamic_listings/python/default/collisiondetection/circle_circle_lazy.py

+        self.center = center
+        self.radius = radius


Missing/Removed Type Hints

Penaz91 · 2024-07-15T12:37:09Z

dynamic_listings/python/default/computer_science/redundancy_1.py

-        nodeptr = self.nodeList
-        while nodeptr:
+        counter: int = 0
+        node = self.node_list[0]


Missing type hint

Penaz91 · 2024-07-15T12:39:59Z

dynamic_listings/python/default/containers/fsm.py

 class FSM:
    """
    This class defines a Finite State Machine
    The currently active state is represented by a function
    pointer
    """

-    current_state = None
+    current_state: Callable = None


Incomplete type hint, from the top of my brain it should be something akin to Callable[[float], None], but maybe it's still valid.

Penaz91 · 2024-07-15T12:40:34Z

dynamic_listings/python/default/containers/fsm.py


-    def setState(self, f):
+    def set_state(self, f: Callable) -> None:


Incomplete type hint, probably Callable[[float], None]

Penaz91 · 2024-07-15T12:43:01Z

dynamic_listings/python/default/containers/timer.py

-    active = False
+    time: float = 0
+    set_time: float = 0
+    function_to_execute: Callable = None


Possibly incomplete type hint (Probably Callable[[], None)

Penaz91 · 2024-07-15T12:44:31Z

dynamic_listings/python/default/collisiondetection/AABB.py

+    is_colliding = (A.corner.x < B.corner.x + B.width) and\
       (A.corner.x + A.width > B.corner.x) and\
       (A.corner.y < B.corner.y + B.height) and\
-       (A.corner.y + A.height > A.corner.y):
-        return True
-    return False
+       (A.corner.y + A.height > A.corner.y)


I think you can omit backslashes in parentheses, it should look cleaner. Good call on stuffing the complex statement in a variable.

…ion detection

dynamic_listings/python/default/optimization/eager_mode.py

Penaz91 · 2024-07-28T18:59:45Z

Hello, I wanted to ask if you are going to add further changes to this merge request. Thank you!

Doublestuf · 2024-07-28T21:44:00Z

I think that'll be all of my changes for now. There are probably a few more type hints that could be added across all of the Python listings, but most of them should be there now.

Penaz91 · 2024-07-29T12:02:36Z

I think that'll be all of my changes for now. There are probably a few more type hints that could be added across all of the Python listings, but most of them should be there now.

Thank you, I'll merge this probably this evening or tomorrow evening and iron out some things here and there. Your contribution has shown me that there are a lot of things that need to be double-checked in the project.

Thank you again for your contribution!

Doublestuf and others added 5 commits July 14, 2024 00:57

In Python: Fixed typos, changed variable and function names to match …

d990d70

…PEP8, and added type hints. About 2/3rds done, will need future revisions.

added myself to CONTRIBUTORS.md

3a92f43

added microStudio to game engine list

e57994a

Merge branch 'develop' into master

c5de4a4

Merge pull request #1 from Doublestuf/patch-1

a4b24c9

added microStudio to game engine list

Penaz91 requested changes Jul 14, 2024

View reviewed changes

Doublestuf added 2 commits July 15, 2024 01:46

made improvements to computer science, the other half of containers, …

2c5b676

…and half of collision detection

Merge branch 'master' of https://github.com/Doublestuf/2DGD_F0TH

28dcd67

Penaz91 requested changes Jul 15, 2024

View reviewed changes

Doublestuf added 2 commits July 15, 2024 20:25

went through the folders AI, algorithms, and the other half of collis…

26ba526

…ion detection

went through 6/14 of the folders for my second pass, mostly done I think

98ec41d

Penaz91 reviewed Jul 19, 2024

View reviewed changes

dynamic_listings/python/default/optimization/eager_mode.py Outdated Show resolved Hide resolved

Doublestuf and others added 2 commits July 20, 2024 02:42

Fixed wrong class name

58004df

Merge branch 'develop' into master

c5d743f

Doublestuf added 2 commits July 28, 2024 17:42

improved collision detection python listings

71ca09d

Merge branch 'master' of https://github.com/Doublestuf/2DGD_F0TH

235ed1b

Penaz91 merged commit c018579 into 2DGD-F0TH:develop Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed typos and made adjustments for PEP8 in Python listings #119

Fixed typos and made adjustments for PEP8 in Python listings #119

Doublestuf commented Jul 14, 2024 •

edited

Loading

Penaz91 left a comment

Penaz91 commented Jul 14, 2024

Doublestuf commented Jul 15, 2024

Penaz91 left a comment

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 Jul 15, 2024

Penaz91 commented Jul 28, 2024

Doublestuf commented Jul 28, 2024

Penaz91 commented Jul 29, 2024


		def setState(self, f):
		def set_state(self, f: Callable) -> None:

Fixed typos and made adjustments for PEP8 in Python listings #119

Fixed typos and made adjustments for PEP8 in Python listings #119

Conversation

Doublestuf commented Jul 14, 2024 • edited Loading

Description

Context and motivation

Type of changes:

Checklist

Penaz91 left a comment

Choose a reason for hiding this comment

Penaz91 commented Jul 14, 2024

Doublestuf commented Jul 15, 2024

Penaz91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Penaz91 commented Jul 28, 2024

Doublestuf commented Jul 28, 2024

Penaz91 commented Jul 29, 2024

Doublestuf commented Jul 14, 2024 •

edited

Loading