Implement type hints for Environment and environment utilities#4824
Implement type hints for Environment and environment utilities#4824bdbaddog merged 3 commits intoSCons:masterfrom
Conversation
|
Tackling a big one, I see :-) This will take a bit to review - and it seems to have blown up a few tests. In |
|
Hmm, I might've left a functional change in there while testing return types; it'd explain the tests acting up anyway. Will take a look later |
fa49a48 to
43f2960
Compare
• Hints operate under the assumption that `Environment.Base` is the "base" class
43f2960 to
adacd48
Compare
|
I asked Gemini for feedback on the type annotations, here's what it came up with. Thoughts? The changes in this PR introduce extensive type annotations across the SCons codebase, particularly in the core environment and node management modules. After Incorrect or Overly Restrictive Type Annotations
Observations and Fixes
Overall, the type annotations are a significant improvement, but adjusting the restrictive hints mentioned above would better reflect SCons's flexible API and |
|
That's weird... the comments in the email show some backward-looking comments in "Observations and Fixes", which has four bullets,,and three of those don't seem to be in the comment in the PR itself, which has one bullet ???? Makes me nervous... |
I think it diffed the branch vs master and since this branch branched before some of the fixes in master presently it analyzed more changes than are in this branch. I removed 3 of the 4 observations and fixed section as they referred to changes in master.. |
okay, that would explain it. thanks. |
|
@Repiteo - can you take a look at the Gemini sugguests and make those changes if they seem sane and/or let me know if it's ok for me to? |
|
Seems like we should have a name for the frequently seen |
|
The numbered observations seem about right to me. |
Add that to sctypes ? |
maybe... but on the other hand, it's a type only for type checking. We had a |
|
@bdbaddog Those are fair observations! While passing nodes/lists in many of these contexts is unorthodox and not really documented, tests indeed exists for basically every case mentioned so it's fair to integrate them @mwichmann iirc, it was removed because the abstraction it provided was able to be substituted in the files themselves by taking advantage of |
Followup to Node type hinting PRs
Hints for Environment are particularly odd, as a lot of cases would technically want to derive from
SubstitutionEnvironmentinstead ofEnvironment.Base. However, I made the type hints act as ifEnvironment.Baseis the base class. This is because, in practice, absolutely nobody is usingSubstitutionEnvironment, and it might even be refactored away in the future(?). The files adjusted were:SCons/Environment.pySCons/Node/__init__.py(Environment→EnvironmentBase)SCons/Node/FS.py(Environment→EnvironmentBase)SCons/Script/SConscript.py(SpecificallySConsEnvironmentand related functions)SCons/Util/envs.py(Utility scripts for environments)Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).