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

Doc Smallfix: Updated the BIDSPath in-place update method example #1026

Merged
merged 9 commits into from Aug 26, 2022
Merged

Doc Smallfix: Updated the BIDSPath in-place update method example #1026

merged 9 commits into from Aug 26, 2022

Conversation

bhvieira
Copy link
Contributor

@bhvieira bhvieira commented Jul 29, 2022

Thanks for contributing. If this is your first time,
make sure to read contributing.md

PR Description

I updated the BIDSPath in-place update example to perhaps make it clearer to users what in-place updating means.
It's a small change, but should make the message clearer.
An out-of-place example could be added using copy() perhaps.

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>"

I updated the `BIDSPath` in-place update example to perhaps make it clearer to users what in-place updating means.
It's a small change, but should make the message clearer.
@welcome
Copy link

welcome bot commented Jul 29, 2022

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

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.

Thanks for the contribution -- in principle I am fine with this -- but note how your change has an impact on the doctests (see failed CI checks):

Due to you removing the "assignment", the .update now prints some information. You would have to update the example to reflect that some output is expected.

See e.g.,

        >>> bids_path.update(acquisition='test', suffix='ieeg',
        ...                              extension='.vhdr', task=None)
       !!!!! here you must put what output is expected !!!!!!
        >>> print(bids_path.basename)
        sub-test_ses-two_acq-test_ieeg.vhdr
        """

@bhvieira
Copy link
Contributor Author

Thanks! (I tried to find tests pertaining to the docs but failed at it). I'll do it as soon as I'm available

@hoechenberger
Copy link
Member

        >>> bids_path.update(acquisition='test', suffix='ieeg',
        ...                              extension='.vhdr', task=None)

The indentation needs to be fixed, too

@sappelhoff
Copy link
Member

@bhvieira do you want to come back to this? Let me know if you need help with anything.

@bhvieira
Copy link
Contributor Author

bhvieira commented Aug 4, 2022

Hi @sappelhoff, yes. I was ill these days, but I should be able to implement the changes shortly

@sappelhoff
Copy link
Member

no rush - get better soon!

@bhvieira
Copy link
Contributor Author

bhvieira commented Aug 8, 2022

I see the current problem is that in mne-stable BIDSPath doesn't infer that datatype is "ieeg", and we get a diff there

@sappelhoff
Copy link
Member

sappelhoff commented Aug 8, 2022

I see the current problem is that in mne-stable BIDSPath doesn't infer that datatype is "ieeg", and we get a diff there

yes, I think that's the case after:

mne_bids/path.py Outdated Show resolved Hide resolved
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #1026 (b03a42a) into main (18999ca) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1026   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files          25       25           
  Lines        3822     3822           
=======================================
  Hits         3639     3639           
  Misses        183      183           
Impacted Files Coverage Δ
mne_bids/path.py 97.67% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sappelhoff
Copy link
Member

Thanks @bhvieira can you please do two more things?

  1. git pull (I have merged upstream changes into your branch)
  2. update CITATION.cff with your name? Please put it right BEFORE Alex Gramfort and use the same structure as other entries (name, affiliation, orcid ... if available)

orcid: 'https://orcid.org/0000-0001-8316-7436'

@sappelhoff sappelhoff added the doc label Aug 26, 2022
@sappelhoff sappelhoff enabled auto-merge (squash) August 26, 2022 13:48
@sappelhoff
Copy link
Member

Sorry for the long silence and thanks for your first contribution @bhvieira

This PR should be auto-merged once all CI checks come back green 🎉

@sappelhoff sappelhoff merged commit 3f77995 into mne-tools:main Aug 26, 2022
@welcome
Copy link

welcome bot commented Aug 26, 2022

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@bhvieira bhvieira deleted the patch-1 branch August 29, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants