Add type hints to main Node script #4650
Conversation
• Adjust NodeTests to account for tweaks in hinting, primarily accounting for truthiness instead of pure types
|
Please leave your PR description to just the function of your PR. We don't really need color commentary on the code. We're well aware, but in general are under resourced. So we're doing the best that we can. |
|
Been nibbling away at it for several years now, there's not only a shortage of bodies to do the work, but on getting refactoring (that seems to quickly get large) through the approval process - with some justification, as "it seems to be working as is" is perhaps a stronger argument than squiggly lines in an IDE or static checker spew. The two biggest current things that are hard to annotate for are Nodes and Environments, which turn up everywhere. And one of the biggest temptations is to do something about one-line getters and setters (which work just fine as is) - so far I've mostly resisted :-). Nodes have some weird inconsistencies that checkers don't like - mismatched signatures (esp. when you get into Alias and Value nodes), inconsistent return types (what should I'll respect @bdbaddog wishes and not go on. |
| @@ -711,30 +711,30 @@ def test_set_always_build(self) -> None: | |||
| node = SCons.Node.Node() | |||
| node.set_always_build() | |||
| assert node.always_build | |||
| node.set_always_build(3) | |||
| assert node.always_build == 3 | |||
There was a problem hiding this comment.
These things are weird, in the code noclean, nocache, precious and always_build are used like bools, but there are some comments about leaving a couple of them as ints, and the unit tests seem to treat them as ints that could have values other than 0/1 (None seems to be possible too in some cases). That's probably not necessary because of the way that code is written - the IDX function the tree printer uses always converts to a bool anyway.
There was a problem hiding this comment.
The comments appear to come from a time before the IDX function was a thing, and no other parts of the code appear to utilize these for anything other than boolean evaluation. The only exception was this test file, but it doesn't seem to actually be to any end; making them evaluate truthiness instead felt more applicable
There was a problem hiding this comment.
The comments appear to come from a time before the
IDXfunction was a thing, and no other parts of the code appear to utilize these for anything other than boolean evaluation. The only exception was this test file, but it doesn't seem to actually be to any end; making them evaluate truthiness instead felt more applicable
perhaps I'll take a whack at converting them in a future PR.
|
Apologies for coming off as hostile; that wasn't my intent. Y'all are obviously just dealing with the hand you've been dealt, the last thing I wanna do is compound it by preaching to the choir |
| @@ -652,7 +659,7 @@ def get_executor(self, create: int=1) -> Executor: | |||
| try: | |||
| act = self.builder.action | |||
| except AttributeError: | |||
| executor = SCons.Executor.Null(targets=[self]) # type: ignore | |||
| executor = SCons.Executor.Null(targets=[self]) # type: ignore[assignment] | |||
There was a problem hiding this comment.
This is an interesting case. While it's currently just suppressing the type-change for the Null executor, this feels like it could just return None instead. It'd be more in-line with how other classes (eg: Scanners) handle their optional arguments/returns. Left it alone because that'd obviously be a huge change, but it's something I've been thinking about
Kept as just the one file for now. I initially wanted to expand to FS as well, but quickly realized that this was a domino effect waiting to happen. I'd rather ensure the core concepts as they currently exist are acceptable, and catch any blunders or oversights as early as possible.
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).