-
-
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
Refactor SConstruct option handling / extract scons options v2 #1137
Conversation
@bryanwweber ... I opened a new PR as the implementation did change a bit. |
0ecd549
to
5b957e2
Compare
5b957e2
to
eba57ae
Compare
eba57ae
to
b0c0a2f
Compare
Never liked the command |
b88ee87
to
6f38147
Compare
A full description of all options is printed for 'scons help --options'.
6f38147
to
b1282c5
Compare
@bryanwweber and @speth ... thanks for the reviews! I believe I took care of everything. Also, thank you @bryanwweber for giving me a crash course on PEP 484. Although I've been using PEP 484 annotations for a while, I was apparently shockingly ignorant about some of the features. What is submitted now, passes Pylance's muster (no complaints on the added sections with PS: ugh ... fixing PEP 484 broke the actual build 🤣 (which for whatever reason still works on my machine ... may be due to a pretty old version of |
d9a9aac
to
5c8b938
Compare
Looks like I was overly aggressive when removing In addition, adding a couple of |
85b09c3
to
099c862
Compare
🎉 ... I think this (finally?) takes care of everything. Found another instance of an issue with type hints (which percolated through a couple of things), but that's now taken care of. @bryanwweber ... all PEP 484 updates are squashed in 2ccda53. As an aside, PS: looks like I jinxed it - and had to force-push yet another time. At this point, I think it is done for real. |
099c862
to
b76e678
Compare
Yeah, this is because SCons has a lot of "magic" things that are automatically imported for you when it runs. It basically has to be addressed at the Pylance level, as far as I can tell, since those things don't get imported in SConstruct/SConscript. |
That’s what I figured but I was hoping that there may be a clever way to account for this without having to tinker with whatever delinter/etc. someone may be using. Regardless, looks like I learned a bit about PEP 484 in this PR … it was definitely worthwhile. In hindsight knowing about some details I know now would have made me avoid a couple of pitfalls. Fanciful helper functions are certainly something to stay away from, but it certainly makes for cleaner code. Also, enabling Pylance’s type hinting capabilities did clarify a couple of things. That said, I think I’m happy with this as it stands … I think that apart from the reST, the added options for |
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.
Looks great @ischoegl! Thanks for the quick efforts to fix this up. It was likewise educational for me to look all this stuff up.
Changes proposed in this pull request
SConstruct
scons help --restructured-text
to create ReST outputscons help
to avoid redundant parsersscons help --option=<name_of_option>
If applicable, fill in the issue number this pull request is fixing
Addresses Cantera/cantera-website#26
If applicable, provide an example illustrating new features this pull request is introducing
or
Further
and
Checklist
scons build
&scons test
) and unit tests address code coverage