d0k3 / GodMode9

GodMode9 Explorer - A full access file browser for the Nintendo 3DS console :godmode:
GNU General Public License v3.0
2.09k stars 190 forks source link

Refactor to improve modularity #809

Closed Nemris closed 1 year ago

Nemris commented 1 year ago

This PR adds documentation and type annotations, and allows the script to be imported as a module.

Additionally, the input file is closed as soon as the data is read and the output file is written to in a single go, while the TRF data structures are populated in memory.

Epicpkmn11 commented 1 year ago

This is my fault but everywhere that says "an TRF" should actually be "a TRF", I copied this from fontriff.py and it seems forgot to fix that whoops, might be a good idea to refactor fontriff.py too if this style is preferred


Personal opinion, feel free to ignore: imo this is... not exactly great? like, it takes 5 times as much code to say the same thing which makes it far less readable to me while also breaking Python 2 compatibility... I'd definitely be for moving the main code to a function called in an if __name__ == "__main__": (as is my more recent style, fontriff.py was a while ago), but I can't say I'm personally a fan of this style.

Nemris commented 1 year ago

Fair point about the style - though around 120 of my lines are just documentation, and a lot of the noise is from type annotations. I see what you mean, though.

On the Python 2 side... that version has been EOL since 2020. Personally, I feel it's time to move on - even if I'm not the one who has the last word here. Even the distro I use has moved all of their officially-supported packages to Python 3 by now.

d0k3 commented 1 year ago

As this is @Epicpkmn11 's original code, @Epicpkmn11 should have the last word in what still needs to be changed. I see there's a lot more comments and documentation now, which is great. I personally don't really care about Python2 compatibility anymore, but there may be good reasons to keep it, and if that can be done with not too much effort, why not?

Also, maybe @Wolfvak and @aspargas2 will want to take a look.

Nemris commented 1 year ago

I can technically just scrap the rewrite and focus on deviating minimally for the original, if preferred. Off the top of my head, the three changes I'd make for sure in that case are to remove the unused import, add the usual main() and limit the scope of the handle to the JSON file. I can also add documentation if needed, but the type hints might have to be skipped if Python 2 compatibility is wanted.

Epicpkmn11 commented 1 year ago

I'd rewrite them like this personally: https://gist.github.com/Epicpkmn11/6869a7862e83c76a630337a1ccc44758

Not necessarily saying that's better, I guess I just don't much like OOP in Python

Edit: On type hints, add them if you want, imo they just make the code look cluttered (and break py2, but not a big deal) and since they're not enforced I don't see the point, but if y'all find them helpful go ahead

Wolfvak commented 1 year ago

I think I already mentioned some of these points in other chats but I'll list them here anyways:

And following up on the message that was just posted: we don't need reusability or the "clarity" that comes with objects, simplicity and readability are the biggest priority here

Nemris commented 1 year ago

Sounds good - should I get started on applying the recommendations or wait for @aspargas2?

Nemris commented 1 year ago

Went ahead and applied some of the recommendations, taking inspiration from @Epicpkmn11's sample. There should be a bit less noise now.

d0k3 commented 1 year ago

Sorry this took me so long. @Epicpkmn11 - do you think this is ready to merge?

Epicpkmn11 commented 1 year ago

Yeah I think this should be good

d0k3 commented 1 year ago

Merged, thank you