Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak Util module #4418

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Tweak Util module #4418

merged 3 commits into from
Sep 22, 2023

Conversation

mwichmann
Copy link
Collaborator

@mwichmann mwichmann commented Sep 19, 2023

Move UtilTests.py from the top level to the Util package directory, for consistency with the other packages with unittests.

Renamed Util/types.py -> Util/sctypes.py. types is the name of a stdlib module and it's a bad idea to duplicate it, even though in this case it was "legal" since our file was not at the top level. (Moving UtilTests.py actually made this a real problem)

Class Selector is no longer an OrderedDict, it just inherits from dict as ordering is now preserved and we never used any extra features of OrderedDict.

Fix API doc build - was missing a good bit of Util since it was split into a package.

Moved the import-loop warning to the top of __init__.py so it will be more visible.

Fiddly linting, doc-stringing, etc. Super-fiddly: pylint flags foo, bar and baz as prohibited variable/function/method names. Actually changed these in UtilTests.py.

SCons.Errors has been a source of import loops because it imports SCons.Util. Now Util is split, directly import from the SCons.Util.sctypes submodule the two things Errors needs - this may reduce the chance of import problems.

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 read the README.rst)
  • I have updated the appropriate documentation

Move UtilTests.py from the top level to the Util package directory,
for consistency with the other packages with unittests.

Renamed Util/types.py -> Util/sctypes.py.  types is the name of a stdlib
module and it's a bad idea to duplicate it, even though in this case it's
"legal" since the file was not at the top level.  (Moving UtilTests.py
actually made this a real problem)

Class Selector is no longer an OrderedDict, it just inherits from dict
as ordering is now preserved and we never used any extra features of
OrderedDict.

Fix API doc build - was missing a good bit of Util since it was split
into a package.

Moved the import-loop warning to the top of __init__.py so it will be
more visible.

Fiddly linting, doc-stringing, etc.  Super-fiddly: pylint flags foo, bar
and baz as prohibited variable/function/method names.  Actually changed
these in UtilTests.py.

SCons.Errors has been a source of import loops because it imports Util.
Now Util is split, directly import from the Util.sctypes submodule the
two things Errors needs - this may reduce the chance of import problems.

Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann mwichmann added the maintenance Tasks to maintain internal SCons code/tools label Sep 19, 2023
@bdbaddog
Copy link
Contributor

One concern here is that out in the wild folks are doing
from SCons.Util.types import is_string or similar instead of doing so from SCons.Util.

So minimally this change should explicitly explained in CHANGES and RELEASE.

I'll do some searching of our more notable users git repos and see what's the situation..

@mwichmann
Copy link
Collaborator Author

mwichmann commented Sep 21, 2023

SCons.Util only became a package in 4.5, before that, there was no SCons.Util.types to import from. Trying to fix my poor choice at the time.

Edit: it seems unlikely that anyone would have picked up importing this in the last six months: it was created such that the symbols continued to be made available just by importing SCons.Util, so there would not have been any "not found" event to trigger someone to go looking. Yes, by looking at the source after seeing the release note you can see that there are now distinct module files in the new package, but I'd be surprised if anyone started using for that reasons.

@bdbaddog bdbaddog merged commit b34fc33 into SCons:master Sep 22, 2023
4 of 5 checks passed
@mwichmann mwichmann added this to the 4.6 milestone Sep 22, 2023
@mwichmann mwichmann deleted the maint/util-cleanup branch September 22, 2023 20:32
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
None yet
Development

Successfully merging this pull request may close these issues.

2 participants