search

Grimmys / rpg_tactical_fantasy_game

A tactical turn-based game project in pygame, open to support
GNU General Public License v3.0
392 stars 85 forks source link

Fix the problem exposed in issue #57 #58

Closed ChristianumSolacium closed 1 year ago

ChristianumSolacium commented 1 year ago

As I exposed in the issue #57, the game crushed because entities was treated as a subscriptable object

Grimmys commented 1 year ago

Good point good point, thanks a lot for the contribution!

Grimmys commented 1 year ago

Tests of Mission class are failing because tests are still relying on the old structure used for entities (dict instead of class).

I will check if I can commit on your branch to fix them.

Grimmys commented 1 year ago

At the end I fixed both the implementation and the test directly on the main branch, it was easier.

Closing this PR since it is covered by commit #950da09536a088a8147872f6ff7d5522adf5955f

ChristianumSolacium commented 1 year ago

that makes sense! it's the first time I contribute to this project (actually my first contribution at all lol), and i am not accustomed yet to it. But compliments for the readability of the code, i loved to read through it ahah Another solution could be to make the class subscriptable, using the getitem method, that maybe could help readability, but it would mean a lot of refactoring

Il giorno mar 2 ago 2022 alle ore 22:06 Grimmys @.***> ha scritto:

At the end I fixed both the implementation and the test directly on the main branch, it was easier.

Closing this PR since it is covered by commit # 950da09536a088a8147872f6ff7d5522adf5955f https://github.com/Grimmys/rpg_tactical_fantasy_game/commit/950da09536a088a8147872f6ff7d5522adf5955f

— Reply to this email directly, view it on GitHub https://github.com/Grimmys/rpg_tactical_fantasy_game/pull/58#issuecomment-1203163144, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARONT42WN2T524WJGIQIBM3VXF5TPANCNFSM55LRHILQ . You are receiving this because you authored the thread.Message ID: @.***>

Grimmys commented 1 year ago

Oh nice, welcome to the contribution world!

Thanks, I try to keep the code as much readable as possible for beginners to be able to read it and take piece of code as clean examples for their projects.

Don't hesitate to contact me if you don't understand anything / if you something should be clarified.

Another solution could be to make the class subscriptable, using the getitem method, that maybe could help readability, but it would mean a lot of refactoring

Hmmm sure, but I don't know if it would even increase readability. Between a.attribute and a["attribute"] there is not much difference...

ChristianumSolacium commented 1 year ago

no for sure, but maybe it could help to understand that it's not a property of the class but a subset. But yes, i am not so sure that does make sense at all ahah

Il giorno mer 3 ago 2022 alle ore 18:25 Grimmys @.***> ha scritto:

Oh nice, welcome to the contribution world!

Thanks, I try to keep the code as much readable as possible for beginners to be able to read it and take piece of code as clean examples for their projects.

Don't hesitate to contact me if you don't understand anything / if you something should be clarified.

Another solution could be to make the class subscriptable, using the getitem method, that maybe could help readability, but it would mean a lot of refactoring

Hmmm sure, but I don't know if it would even increase readability. Between a.attribute and a["attribute"] there is not much difference...

— Reply to this email directly, view it on GitHub https://github.com/Grimmys/rpg_tactical_fantasy_game/pull/58#issuecomment-1204190419, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARONT425MLK6FOV3ZY43PE3VXKMPJANCNFSM55LRHILQ . You are receiving this because you authored the thread.Message ID: @.***>

Grimmys commented 1 year ago

Well technically, it's an attribute.

It just that this class is only an aggregate of collections, not really a class like we use to see them.