Make sure consistently using one docstring style by mwichmann · Pull Request #4822 · SCons/scons

mwichmann · 2026-01-28T18:30:48Z

The remaining files that have old-style aka reST-style docstrings are switched to Google style. This does not remove the inline reST-style annotations for functions, classes, modules, etc. - that's a different topic, we want to keep those so API docs are generated with hyperlinks to the referenced element.

In a bit of "burying the lede"... there was an actual bugfix here, in Defaults.py a copy-pasted chunk ended up with a reference to a variable that was not in scope at that point (in processDefines). Was glancing at Defaults.py in an IDE to see what it thought was broken, and it was red on that one.

Some minor de-linting in Defaults.py and Node/FS.py, and fix an annotation error in FS.py (invalidate_node_memos) - of course that one has no runtime effect.

This has no doc or test changes, it's nearly all docstrings, type annotations and moving imports - none of which have runtime effects.

Contributor Checklist:

I have created a new test or updated the unit tests to cover the new/changed functionality.
I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
I have updated the appropriate documentation

The remaining files that have old-style or reST-style docstrings are switched to Google style. This does *not* remove the inline reST-style annotations for functions, classes, modules, etc. - that's a different topic, we want to keep those so API docs are generated with hyperlinks to the referenced element. Burying the lede... there was an actual bugfix here, in Defaults.py a copy-pasted chunk ended up with a reference to a variable that was not in scope at that point (in processDefines). Was glancing at Defaults.py in an IDE to see what it thought was broken, and it was red on that one... Some minor de-linting in Defaults.py and Node/FS.py, and fix an annotation error in FS (invalidate_node_memos) - of course that one has no runtime effect. Signed-off-by: Mats Wichmann <mats@linux.com>

Repiteo

The returning None on failure, 1 on success. makes my heart ache, but I understand this is a non-functional pass. However, seeing as you are updating the type hints, the container types should be explicit

Repiteo · 2026-02-01T17:38:31Z

testing/framework/TestCmd.py

+    lines: str | list | None = None,
+    matches: str | list | None = None,


Hmmm, this isn't going to work as annotated anyway... if lines is the default None, then:

if not is_List(lines): lines = lines.split(newline)

is going to throw an AttributeError cause None has no split method. The same for matches, for that matter. I'm thinking a default of an empty string is better?

Repiteo · 2026-02-01T17:38:38Z

testing/framework/TestCmd.py


-def match_caseinsensitive(lines=None, matches=None):
+def match_caseinsensitive(
+    lines: str | list | None = None, matches: str | list | None = None


Same problem as for match_exact

Repiteo · 2026-02-01T17:39:04Z

testing/framework/TestCmd.py



-def match_re(lines=None, res=None):
+def match_re(lines: str | list | None = None, res: str | list | None = None) -> int | None:


Same problem as for match_exact

Repiteo · 2026-02-01T17:39:41Z

testing/framework/TestCmd.py


-def match_re_dotall(lines=None, res=None):
+def match_re_dotall(
+    lines: str | list | None = None, res: str | list | None = None


Repiteo · 2026-02-01T17:39:52Z

testing/framework/TestCmd.py

            do_chmod(top)

-    def write(self, file, content, mode: str = 'wb'):
+    def write(self, file: str | list, content: str | bytes, mode: str = 'wb'):


Suggested change

def write(self, file: str | list, content: str | bytes, mode: str = 'wb'):

def write(self, file: str | list[str], content: str | bytes, mode: str = 'wb'):

mwichmann · 2026-02-01T17:54:04Z

The returning None on failure, 1 on success. makes my heart ache

Indeed.

mwichmann · 2026-02-01T18:46:23Z

These look fine to me, @bdbaddog okay for the changes to be added?

bdbaddog · 2026-02-02T01:53:29Z

These look fine to me, @bdbaddog okay for the changes to be added?

My type hints fu is weak. If they look good you, go ahead..

For the match functions that can take a list argument, annotate then as string-or-list-of-strings. (from review comment) Also the write method - the "filename" can be a list of strings (path components), annotate is as such as well. Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann · 2026-02-03T00:30:18Z

So.. should we eliminate the None type option and None default for the observed and expected arguments to the four match functions? As mentioned above, they will raise an exception if None actually made it through as an argument value, nor do they make any actual sense - how can you call a function to compare two strings or lists of string (or regex strings) if one or both aren't even supplied?

SCons/Defaults.py

…hanges in that file

mwichmann added documentation maintenance Tasks to maintain internal SCons code/tools labels Jan 28, 2026

Repiteo reviewed Feb 1, 2026

View reviewed changes

mwichmann force-pushed the maint/docstring-gs branch from f29ab44 to 2e1acdf Compare February 3, 2026 00:09

bdbaddog reviewed Feb 6, 2026

View reviewed changes

SCons/Defaults.py Show resolved Hide resolved

revert compilation_db.py to avoid conflicting PR 4823 which has big c…

cdb6fc7

…hanges in that file

bdbaddog merged commit 50697c6 into SCons:master Feb 9, 2026
10 of 11 checks passed

mwichmann added this to Next Release Feb 9, 2026

github-project-automation bot moved this to Done in Next Release Feb 9, 2026

mwichmann added this to the NextRelease milestone Feb 9, 2026

mwichmann deleted the maint/docstring-gs branch February 9, 2026 15:38

		lines: str \| list \| None = None,
		matches: str \| list \| None = None,

	lines: str \| list \| None = None, matches: str \| list \| None = None
	lines: str \| list[str] \| None = None, matches: str \| list[str] \| None = None



		def match_re(lines=None, res=None):
		def match_re(lines: str \| list \| None = None, res: str \| list \| None = None) -> int \| None:

	lines: str \| list \| None = None, res: str \| list \| None = None
	lines: str \| list[str] \| None = None, res: str \| list[str] \| None = None

	def write(self, file: str \| list, content: str \| bytes, mode: str = 'wb'):
	def write(self, file: str \| list[str], content: str \| bytes, mode: str = 'wb'):

Uh oh!

Comments

Conversation

mwichmann commented Jan 28, 2026

Contributor Checklist:

Uh oh!

Repiteo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwichmann commented Feb 1, 2026

Uh oh!

mwichmann commented Feb 1, 2026

Uh oh!

bdbaddog commented Feb 2, 2026

Uh oh!

mwichmann commented Feb 3, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Repiteo left a comment •

edited

Loading