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

MAINT: Encapsulate config import #635

Merged
merged 14 commits into from
Oct 21, 2022
Merged

Conversation

larsoner
Copy link
Member

This PR moves toward simplification of config.py by:

  1. Doing a lot of copy-paste to _whatever.py files from config.py (looks like ~1700 lines removed from config.py)
  2. Removing all import config / from config import from the top of scripts in favor of relative imports of from ..._whatever import ...
  3. Fixing a number of bugs with broken encapsulation demonstrated by (2)
  4. Removes the workarounds present in docs that are no longer needed

We'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.

@larsoner larsoner mentioned this pull request Oct 15, 2022
14 tasks
@larsoner
Copy link
Member Author

Ready for review/merge from my end

@hoechenberger
Copy link
Member

@larsoner This will need a merge / rebase on main once #634 has been merged

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 :)

@agramfort
Copy link
Member

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,
Copy link
Member

@hoechenberger hoechenberger Oct 16, 2022

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.

Copy link
Member Author

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

@hoechenberger
Copy link
Member

@larsoner I think this is good to merge from my end now, thank you. but please hold off for now if possible so I can get #625 finalized first

@larsoner
Copy link
Member Author

Works for me

@larsoner
Copy link
Member Author

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!

@larsoner larsoner merged commit 12973e8 into mne-tools:main Oct 21, 2022
@larsoner larsoner deleted the encapsulate branch October 21, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants