-
Notifications
You must be signed in to change notification settings - Fork 71
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
MAINT: Encapsulate config import #635
Conversation
Ready for review/merge from my end |
@larsoner This will need a merge / rebase on Also I'd kindly ask to hold off merging this until I've finished working on #625 – it's my fourth (or so) attempt to get CSP into the pipeline, and I've had to spend so much time rebasing / restarting because upstream changes kept on piling up … I don't want to have to do this again! So please give me a chance to get #625 finished before we proceed here :) |
looks good ! @hoechenberger what's the status on CSP? Can I help? |
@@ -49,7 +47,7 @@ def init_subject_dirs( | |||
*, | |||
cfg, | |||
subject: str, | |||
session: Optional[str] = None | |||
session: str, |
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.
@larsoner Why did you remove the Optional
type in many places?
Maybe there's a (common) misunderstanding here: Optional[foo]
is actually shorthand for Union[None, foo]
, which can (finally!!) be written as None | foo
in Python 3.10 (this was sooo overdue).
You can, therefore, have a required parameter that is nevertheless annotated with an Optional
type if it's allowed to be None
.
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.
Ahh yes whatever can be None can be restored then. Or maybe we should just wait until 3.10 is the minimum requirement and use the nicer syntax. Up to you
Works for me |
Now that #625 is working and I've seen what's required, I'll merge this and fix any conflicts. I don't think they'll be too bad! |
This PR moves toward simplification of
config.py
by:_whatever.py
files fromconfig.py
(looks like ~1700 lines removed fromconfig.py
)import config
/from config import
from the top of scripts in favor of relative imports offrom ..._whatever import ...
docs
that are no longer neededWe'll see how much stuff fails. Once it's green I think we can merge. Then hopefully just one more PR will be necessary to move the "input validation" stuff out of
config.py
that is left at the end of the file.