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

[MRG] Make _parse_bids_filename() public, drop verbose arg #487

Merged
merged 5 commits into from Aug 5, 2020

Conversation

hoechenberger
Copy link
Member

PR Description

We make use of mne_bids.path._parse_bids_filename() in the mne-study-template in a totally un-hacky manner, making us believe that this function should probably be made public.

This PR also drops the function's verbose argument, as it was unused.

Describe your PR here

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"
  • Commit history does not contain any merge commits

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me, actually cool when a private func turns out to be so useful. Why did it have a verbose arg before?

And I also like the added "examples" section to the docstr 👍

We make use of `mne_bids.path._parse_bids_filename()` in the
mne-study-template in a totally un-hacky manner, making us believe
that this function should probably be made public.

This commit also drops the function's `verbose` argument, as it
wasunused.
@@ -65,6 +65,7 @@ API
- :func:`mne_bids.make_dataset_description` now takes the argument `overwrite` which will reset all fields if `True`. If `False`, user-provided fields will no longer be overwritten by :func:`mne_bids.write_raw_bids` when its `overwrite` argument is `True` unless new values are supplied, by `Alex Rockhill`_ (`#478 <https://github.com/mne-tools/mne-bids/pull/478>`_)
- :func:`mne_bids.make_report` is now available from the `mne_bids` namespace that creates a string output of a summary of the BIDS dataset. In addition, the command line interface allows one to call `make_report`, by `Adam Li`_ (`#457 <https://github.com/mne-tools/mne-bids/pull/457>`_)
- Added namespace :code:`mne_bids.path` which hosts path-like functionality for MNE-BIDS by `Adam Li`_ (`#483 <https://github.com/mne-tools/mne-bids/pull/483>`_)
- :func:`mne_bids.path._parse_bids_filename` is now part of the public API and has consequently been renamed to `:func:`mne_bids.path._parse_bids_filename`, by `Richard Höchenberger`_ (`#487 <https://github.com/mne-tools/mne-bids/pull/487>`_)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private functions cannot be put in the what's new as they are not part of the public API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also need to add to API rst page the new public function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private functions cannot be put in the what's new as they are not part of the public API

But it is now… so it's a "new" function. Do you mean I should not list it under "API", but instead under Changelog?

you also need to add to API rst page the new public function.

will do

@agramfort
Copy link
Member

agramfort commented Aug 5, 2020 via email

@hoechenberger
Copy link
Member Author

hoechenberger commented Aug 5, 2020

you wrote _parse_bids_filename

I think I see what you mean!

@codecov-commenter
Copy link

Codecov Report

Merging #487 into master will not change coverage.
The diff coverage is 85.71%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #487   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files          14       14           
  Lines        1970     1970           
=======================================
  Hits         1827     1827           
  Misses        143      143           
Impacted Files Coverage Δ
mne_bids/read.py 95.43% <60.00%> (ø)
mne_bids/dig.py 90.71% <100.00%> (ø)
mne_bids/path.py 96.37% <100.00%> (ø)
mne_bids/report.py 91.17% <100.00%> (ø)
mne_bids/write.py 96.74% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 149204d...cda0bb7. Read the comment docs.

@hoechenberger hoechenberger changed the title Make _parse_bids_filename() public, drop verbose arg [MRG] Make _parse_bids_filename() public, drop verbose arg Aug 5, 2020
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, thanks @hoechenberger !

@jasmainak jasmainak merged commit fd2fa56 into mne-tools:master Aug 5, 2020
@hoechenberger hoechenberger deleted the parse_bids_filename branch August 5, 2020 17:01
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.

None yet

5 participants