ClanGenOfficial / clangen

Warrior Cats fan game
https://clangen.io
Other
229 stars 403 forks source link

[ENHANCEMENT] Linting #2395

Closed j-gynn closed 4 weeks ago

j-gynn commented 1 month ago

About The Pull Request

This is a PR to integrate some code cleanup work I've been doing. The goal is to ultimately have (as close as possible to) zero PyLint warnings on every file in the entire scripts folder... but that's a looong way away!

Broad overview of changes

As it stands, there have been NO file/structural changes. However, I strongly believe some of the code in cats.py should not be in that file (looking STRONGLY at generate_lead_ceremony, for example). That said, I don't think I can nor should make that call. If there's a way to present a proposal to the senior devs/leads then please let me know and I will write something up.

I will not be touching anything related to moonskip events because I know that is having a major overhaul right now. Please advise if there's anything else that there's no point working on due to upcoming major changes!

Below, you will find an assortment of bugs I've found just by reading the code and logicifying - I will add them as separate bug reports, Soon (tm).

Bugs found

Why This Is Good For ClanGen

Code maintenance is essential for well-running projects, especially ones with lots of developers. It helps keep code accessible & understandable to those new to the base; running efficiently for end-users; easy to expand upon for the senior team, etc.

Linked Issues

Related to a previous PR #2382 , which is why I have made this draft PR. @susieoneil - please feel free to collab on this repo if you want, or alternatively I can merge into yours if you've made significant progress, as I know you said you were going to start tackling the scripts folder in the future. If you can't do that, I can manually give you access - let me know.

Proof of Testing

No features have been added. Game opens, loads save and runs multiple moonskips with no issues.

Changelog/Credits

Linting - j-gynn

scribblecrumb commented 1 month ago

Love the look of this! Honestly a godsend to have someone willing to go through the existing code and clean it up. I will mention that I've got plans to refactor all the Screens scripts as they're in desperate need of some TLC. I've not started on that yet though, but if you end up looking those over and have ideas for streamlining then I'll be happy to hear them.

I've also made you an apprentice dev on the discord! You'll have access to the dev section and we can talk more there :+1:

j-gynn commented 1 month ago

I will make a new PR for refactoring work I wanna do, just so we can get the linting done sooner rather than later. One note - I have updated the .gitignore - for some reason it had both \ and / variants for some folders and it was throwing errors for black (the formatter this now uses).

susieoneil commented 1 month ago

Wow, you have been busy! Glad my idea was taken on board.

j-gynn commented 1 month ago

I think... this might be passing tests now? As an aside, you can actually run tests locally now using the command line and poetry run pytest so that's a nice bit of QoL