-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Avoid star import from buildutils #1791
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1791 +/- ##
=======================================
Coverage 73.23% 73.24%
=======================================
Files 381 381
Lines 54375 54375
Branches 9251 9251
=======================================
+ Hits 39823 39826 +3
+ Misses 11580 11578 -2
+ Partials 2972 2971 -1 ☔ View full report in Codecov by Sentry. |
from buildutils import (Option, PathOption, BoolOption, EnumOption, Configuration, | ||
logger, remove_directory, remove_file, test_results, | ||
add_RegressionTest, get_command_output, listify, which, | ||
ConfigBuilder, multi_glob, quoted, add_system_include, | ||
checkout_submodule, check_for_python, check_sundials, | ||
config_error, run_preprocessor, make_relative_path_absolute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this one is actually better than the *
. Otherwise, the rest look good to me 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Citing from PEP 8:
Wildcard imports (
from <module> import *
) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API (for example, overwriting a pure Python implementation of an interface with the definitions from an optional accelerator module and exactly which definitions will be overwritten isn’t known in advance).
The alternative would be to use import buildutils as bu
etc., but I honestly think that the long list at the beginning of the module is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm familiar with PEP 8. I think in this case, it makes more sense to ignore pep 8's recommendation. I should explain my reasoning.
In this case, I think this long block is less readable and maintainable than the *. I feel that way because, well , readability is just vibes as the kids say. But I do think it's harder to maintain this long list here rather than in the dunder all in buildutils.
There are a couple of other reasons that pep 8 makes that recommendation against * (although they're not stated explicitly), which I think don't apply here.
- I also considered that the * import can't be resolved by the LSP implementation. However, even the explicit import can't be resolved because
site_scons
is added to the path by SCons at runtime. - Similarly, I don't think naming everything on the import makes it more obvious where they come from because buildutils is hidden inside site_scons.
- I also considered namespace pollution from buildutils. Since we define dunder all in buildutils, we are essentially defining what should be exported rather than imported. In any case, this avoids the pollution issue since we use nearly every exported member in the main SConstruct.
- SCons also does a bunch of magical injections of objects at runtime that you can use without importing. So in that sense using a * fits in the same pattern as SCons is already using.
So, considering all that, my opinion is that the * is the better tradeoff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the point of a maintainer, I can probably see your points. I do, however, believe that we should approach this from the perspective of users who are not familiar with SCons and other intricacies, all of which have a steep learning curve. The fact that SCons makes a couple of design decisions that are debatable (points 1/2/4 - site_scons
and other magic injections) is imho not a reason to throw Python guidelines overboard.
Based on clarity, PEP8 clearly wins, as someone who stumbles across any of the custom utils, - say add_system_include
, - would presumably search for this in SConstruct
but would not find where it is defined if it is imported via from buildutils import *
unless they searched across the entire Cantera source code. Using VSCode, Pylance complains as follows:
"add_system_include" is not defined
so anyone not searching across all of Cantera won't be able to find it. If we use the list, they would at least know that it's from buildutils
and not part of something magically injected by SCons. While Pylance still doesn't find the definition with the list import, it no longer complains.
Fwiw, the __all__
in buildutils.py
- point 3 - is similarly cumbersome to maintain (and should probably be removed as part of this PR). Edit: pushed a commit that will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're approaching the problem with the same facts and just prioritizing differently. We may have to agree to disagree and call in @speth to break the tie 😁
Changes proposed in this pull request
Replace
with imports of specific methods and classes. The change improves readability and avoids unnecessary clutter in various namespaces.
Checklist
scons build
&scons test
) and unit tests address code coverage