Skip to content

Conversation

@teonbrooks
Copy link
Member

PR Description

I noticed that when using the BIDSPath and having passed the argument check=False, the value wasn't being passed to the underlying update function.

I noticed this when I was passing the argument datatype='eyetrack' (not supported by BIDS validation) but with the check=False. when it came time to use BIDSPath.fpath, it errored

Within the update method, it does a self-assign to check, and since the check argument isn't passed, it defaults to True

In [1]: bids_path = BIDSPath(root=bids_root, session=None, task=task,
    ...:                      datatype='eyetrack', check=False)

In [2]: bids_path.fpath
...
ValueError: datatype (eyetrack) is not valid. Should be one of ['meg', 'eeg', 'ieeg', 'nirs', 'anat', 'beh']

In [3]: bids_path.check
Out[3]: False

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

@codecov
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.45%. Comparing base (a619cb7) to head (b2253d3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1411   +/-   ##
=======================================
  Coverage   97.45%   97.45%           
=======================================
  Files          40       40           
  Lines        9088     9093    +5     
=======================================
+ Hits         8857     8862    +5     
  Misses        231      231           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hoechenberger
Copy link
Member

hoechenberger commented Jun 5, 2025

Hey Teon, good to see you again 👍 I hope things are going well for you.

I think we'll need both lines here – the one you removed and the one you added – to make things work as documented (docs say that update(check=...) will set the .check attribute)

Care to update?

@hoechenberger hoechenberger requested a review from sappelhoff June 5, 2025 13:43
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Actually, disregard my previous comment, I think your proposed fix is exactly right. I got confused between the two methods!

@larsoner
Copy link
Member

larsoner commented Jun 5, 2025

Hi @teonbrooks 👋 !!!

... would be great to have a test that fails on main and passes on this PR 😄

@teonbrooks
Copy link
Member Author

so sorry, where are my manners?! hiya! 👋🏾 good to see you both 😄

@teonbrooks
Copy link
Member Author

... would be great to have a test that fails on main and passes on this PR 😄

cool! i'll get on that. I'll update the title to WIP as a reminder. I should have some time to get to it tomorrow (maybe today if I'm lucky)

@teonbrooks teonbrooks marked this pull request as draft June 5, 2025 16:39
@sappelhoff
Copy link
Member

Hi Teon 👋 thanks for taking care of this 🚀

teonbrooks and others added 2 commits June 15, 2025 15:03
also within `_get_matching_bidspaths_from_filesystem`, the check parameter was being passed so it would not work when trying to generate a fpath

Added test to check for the fpath creation with datatype not in VALID_DATATYPES when check=False
@teonbrooks teonbrooks marked this pull request as ready for review June 15, 2025 19:04
@teonbrooks teonbrooks changed the title [BUG] Check isn't being passed to update function [MRG] Ensure check is passed to update function Jun 15, 2025
@teonbrooks
Copy link
Member Author

Got around to it this afternoon. If someone could give it a quick review, I would appreciate it :)

@larsoner larsoner merged commit cc0fc70 into mne-tools:main Jun 16, 2025
24 checks passed
@larsoner
Copy link
Member

Thanks @teonbrooks !

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.

4 participants