search

encounter / dtk-template

Project template for decomp-toolkit
Creative Commons Zero v1.0 Universal
17 stars 14 forks source link

Implement mypy fixes across all python files #4

Closed Repiteo closed 8 months ago

Repiteo commented 9 months ago

Part of figuring out what is causing inconsistencies with my prior PR was needing a more concrete and unambiguous foundation to work with. To that end, this PR adds a ton of adjustments to the various tool scripts. Most prominently: type hints are now available on every single file. While this did occasionally get a bit wordy (shoutouts to NinjaPaths in ninja_syntax.py), this ultimately made the expected input and output a lot more parsable

Several functions were adjusted in this process, albeit without changing the fundamental intent. There is, however, one primary exception to this: passing paths can now be done directly! The conversion process will now be handled in ninja_syntax.py natively, so the streamlined Path syntax can be freely used in project.py without worry. What's more, because of the Union functionality, strings and paths can be passed interchangably, either in isolation or as a collection, and the ninja builder just handles it

ribbanya commented 9 months ago

It looks like basic support was introduced in 3.5; not sure about the minimum version of these changes.

If you like, I could externalize the type hints to stub files, similar to how typescript adds types to legacy javascript libraries, which would address both the minimum version and the presence of changes to ninja_syntax.py.

Repiteo commented 9 months ago

Having given this more thought, it should be possible to leave ninja_syntax.py unchanged (minus hinting) & have the conversions occur before getting passed down. I'll test that locally and see if we can retain the benefits of added readability

Repiteo commented 9 months ago

Hmm, well it works, but there's quite a lot of Path used in project.py, so there's two main options I would consider:

  1. Bite the bullet and just let ninja_syntax.py be custom-tailored to this repo
  2. Take inspiration from the ninja repo & swap out Path for os.path equivalents; this would keep everything string-based
encounter commented 8 months ago

Looks great as-is! Thanks for the PR.

Will have to update requirements to say Python 3.8+. Hopefully not too many people are running older versions...

ribbanya commented 8 months ago

If it helps, 3.7 reached EOL last year.