-
Notifications
You must be signed in to change notification settings - Fork 90
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
MRG: Use hatchling as build backend #1204
Conversation
Also dynamically generate version number via hatch-vcs Addresses mne-tools#1174
c45dbfc
to
fdbf6eb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
- Coverage 97.63% 97.61% -0.03%
==========================================
Files 40 40
Lines 8681 8685 +4
==========================================
+ Hits 8476 8478 +2
- Misses 205 207 +2 ☔ View full report in Codecov by Sentry. |
doc/whats_new.rst
Outdated
- nothing yet | ||
- The package build backend has been switched from ``setuptools`` to ``hatchling``. This | ||
only affects users who build and install MNE-BIDS from source, and should not lead to | ||
changed runtime behavior, by `Richard Höchenberger`_ (:gh:`1204`) |
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.
in what ways does it affect users who build and install mne-bids from source? :)
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.
The build process is simply different now. Maybe it should be phrased differently? No user intervention is required
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 am not sure it should be in the release notes then🤔 usually we skip "infrastructure changes without effect on the user", don't we?
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.
There should be a Code Health section like we have in MNE-BIDS-Pipeline.
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.
pyproject.toml
Outdated
@@ -80,7 +74,7 @@ doc = [ | |||
"mne-nirs @ https://github.com/mne-tools/mne-nirs/archive/refs/heads/main.zip", | |||
"seaborn", | |||
"openneuro-py", | |||
"defusedxml", # for reading BrainVision montages: `examples/convert_eeg_to_bids.py` | |||
"defusedxml", # for reading BrainVision montages: `examples/convert_eeg_to_bids.py` |
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.
is this is mistake or by design? if by design, why?
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.
My linter adds this automatically... but I can change it
Otherwise number of spaces before an inline comment in TOML is just 1
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.
Btw the linter I use comes with the Even Better TOML extension for VS Code
Very helpful tool really
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.
in general, I think this is nice -- thanks for adding it!
It would also need updates to the release protocol in the WIKI
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.
+1 to merge if:
- the changelog entry is moved to a new, adequate "codehealth" section
- this is resolved: https://github.com/mne-tools/mne-bids/pull/1204/files#r1419479479
We can update the release protocol separately.
Thanks!!
Thanks |
@sappelhoff I think we're good to go |
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.
LGTM
Also dynamically generate version number via hatch-vcs
Addresses #1174
cc @ofek
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: