Merged
Conversation
Fix various niggly typing and checker complaints. Environment.py: * To avoid modifying a dict-like thing while iterating over it, we did .copy(), making an awkward chained construct that checkers don't like. Simpler (and the more common idion) is to make a list out of the .keys() view, that way you're iterating over that new list and not the dict (and save creating a dict copy). * A few complaints about calling subst with a bad type were due to us defeating type narrowing - checkers don't know that is_String evaluating true means the object is a string type. Added casts for some of these. * Another use of copy() was in detecting a CPPDEFINES deque, where we want to keep a copy of it and not modify the original - use the deque constructor for this instead of calling copy() method. * Align the names of vars in overridden dunder methods in BuilderDict with parent UserDict for more consistency. * arg2nodes node_factory argument *could* be passed a None, indicate that in the signature. * Multiple uses of CacheDir (it was both an import from CacheDir for type checking, and the name of a function in Environment.py). This was confusing to at least one type checker, so just use the qualified name. * The environment Base class _update and _update_onlynew methods were annotated to take an EnvironmentBase argument, but in fact they're designed to update the internal dict from a passed dict. Adjusted the annotation. * Used the global Environment hook directly by name rather than fully qualified, as SCons.Environment doesn't have an import of itself. That's namespace niggling, because it worked as written. * The Repository method (public API level) was written to take kwargs and pass those on to the FS layer's Repository - which doesn't take kwargs. Repository() was never documented as accepting any keyword arguments (and no tests ever poked at that usage), so just removed to align the two. * In VariantDir, make a small readability change and also avoid reassigning the function parameters: the index-zero access occurs on the call to self.fs.VariantDir, not on the end of the call to arg2nodes. Node/FS.py: Glob and glob annotation adjusted to show the exclude argument could also be a single string, as that could be passed down from the public API level, as documented: "The optional exclude argument may be set to a pattern or a list of patterns describing files or directories to filter out of the match list." Tool/FortranCommon.py: fix an error where returns were Tuple, which is not imported; changed to tuple. Add more typing casts to Environment A few complaints about calling subst with a bad type were due to us defeating type narrowing - checkers don't know that is_String evaluating true means the object is a string type. Added casts for certain instances of this. Signed-off-by: Mats Wichmann <mats@linux.com>
fc2a9ba to
ca7aaa5
Compare
Repiteo
approved these changes
Mar 27, 2026
Contributor
Repiteo
left a comment
There was a problem hiding this comment.
Everything seems to check out!
Will need RELEASE.txt updated, though
Collaborator
Author
I dodged it as there are already three entries there that collectively say Environment was tweaked for typing and docstrings. I figure it's covered... will wait to hear. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix various niggly typing and checker complaints.
Environment.py:.copy(), making an awkward chained construct that checkers don't like. Simpler (and the more common idiom) is to make a list from the keys view, that way you're iterating over that new list and not the dict (and save creating a dict copy).copy()was in detecting aCPPDEFINESdeque, where we want to keep a copy of it and not modify the original - use the deque constructor for this instead of calling copy() method.substwith a bad type were due to us defeating type narrowing - checkers don't know thatis_Stringevaluating true means the object is a string type. Added casts for certain instances of this.BuilderDictwith parentUserDictfor more consistency (one of ours used the same name as a different arg in the parent). Similar for theJSONEncoderdefaultmethod.arg2nodesnode_factoryargument could be passed aNone, indicate that in the signature.CacheDir(it was both an import fromCacheDirfor type checking, and the name of a function inEnvironment.py). This was confusing to at least one type checker, so just use the qualified name.Baseclass_updateand_update_onlynewmethods were annotated to take anEnvironmentBaseargument, but in fact they're designed to update the internal dict from a passed dict. Adjusted the annotation.Environmenthook directly by name rather than fully qualified, asSCons.Environmentdoesn't have an import of itself. That's namespace niggling, because it worked as written.Repositorymethod (public API level) was written to take kwargs and pass those on to the FS layer'sRepository- which doesn't take kwargs.Repository()was never documented as accepting any keyword arguments (and no tests ever poked at that usage), so just removed to align the two.VariantDir, make a small readability change and also avoid reassigning the function parameters: the index-zero access occurs on the call toself.fs.VariantDir, not on the end of the call toarg2nodes.Node/FS.py:Globandglobannotation adjusted to show the exclude argument could also be a single string, as that could be passed down from the public API level, as documented: "The optional exclude argument may be set to a pattern or a list of patterns describing files or directories to filter out of the match list."Tool/FortranCommon.py: fix an error where returns were annotatedTuple, which is not imported; changed totuple.Util/__init__.py: use a typevar forsemi_deepcopy, which is a dispatch function, type checkers don't seem to read through to the return types of the various copy functions it calls.No test changes - for the small code changes, we want to show behavior didn't change.
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).