Skip to content

Conversation

@mwichmann
Copy link
Collaborator

This is a no-code change (1) - addition or modification of docstrings, adding blank lines for readability, fixing spacing between code and comment, and a few typing annotations.

  1. there are a few instances of else dropped or elif turned to if when the previous block ended with return or raise. Those are technically code changes, but have no behavioral effect.

@mwichmann mwichmann added the maintenance Tasks to maintain internal SCons code/tools label Nov 15, 2025
SCons/Subst.py Outdated
A substitution mini-language describes how SCons performs token
replacement on strings or lists of strings that are intended for use
in commands. ``${expression}`` is the primary format. ``expression`` can
Copy link
Contributor

@bdbaddog bdbaddog Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not strictly true. in most cases $VARIABLE is the preferred version. ${VARIABLE} causes more complicated processing to happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean preferred, just that it's the baseline - everything works with {}, only some things can omit it. I'll see if there's another way to word that.

Copy link
Contributor

@bdbaddog bdbaddog Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say $ENVVAR is the primary format if you can't use that due to strings following it or because you have an expression you want it to evaluate, otherwise use ${expression} ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say $ENVVAR is the primary format if you can't use that due to strings following it or because you have an expression you want it to evaluate, otherwise use ${expression} ?

I'd be more inclined to keep wordsmithing this if it was for the manpage (something I think we've done already). Just wanted to have a block in the module that said something about the overall purpose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have ${expression} as the primary format. There're performance implications on that form vs $ENVVAR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it says:

If expression is a construction variable reference, the braces can be omitted (this is preferred for performance reasons)

@mwichmann
Copy link
Collaborator Author

I can invert it.

@mwichmann mwichmann moved this to In progress in Next Release Nov 16, 2025
@bdbaddog
Copy link
Contributor

I can invert it.

That'd resolve my concerns. Thanks!

@mwichmann
Copy link
Collaborator Author

I can invert it.

That'd resolve my concerns. Thanks!

Didn't seem to flow actually "inverted", but a new edit coming through.

@mwichmann mwichmann force-pushed the maint/subst-docstrings branch 2 times, most recently from eb940d9 to a7fc4df Compare November 21, 2025 17:22
@mwichmann mwichmann moved this from In progress to In review in Next Release Nov 23, 2025
This is a no-code change (1) - addition or modification of docstrings,
adding blank lines for readability, fixing spacing between code and
comment, and a few typing annotations.

1. there are a few instances of else dropped or elif turned to if
when the previous block ended with return or raise.  Those are
*technically* code changes, but have no behavioral effect.

Signed-off-by: Mats Wichmann <[email protected]>
Two string containers defined here, Literal and SepcialAttrWrapper,
do not subclass UserString. Add a note on that for uses which might
expect them to be identifiable as string types.  CmdStringHolder does
subclass UserString, so it doesn't have this caveat.

Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann mwichmann force-pushed the maint/subst-docstrings branch from a7fc4df to fd250e5 Compare November 27, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Tasks to maintain internal SCons code/tools

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

2 participants