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

skip mac sidecars #470

Merged
merged 11 commits into from
May 30, 2024
Merged

Conversation

pauladkisson
Copy link
Collaborator

fixes #469

@CodyCBakerPhD
Copy link
Contributor

Also CHANGELOG and simple test? (take one of the existing simple tests for an invalid file, make a copy of that file with the prefix, call that function on that directory and ensure it only reports issues with the non-sidecar)

@CodyCBakerPhD
Copy link
Contributor

And also looks like you'll need to run pre-commit locally, since contributing from an organizations fork (the one exception where the pre-commit bit doesn't run)

@@ -103,6 +103,8 @@ def setUpClass(cls):
# Last file to be left without violations

cls.nwbfile_paths = [str(cls.tempdir / f"testing{j}.nwb") for j in range(num_nwbfiles)]
nwbfiles.append(make_minimal_nwbfile())
Copy link
Contributor

Choose a reason for hiding this comment

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

A minimal NWB file would have no problems with it though

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe bump the number of files to 4, do one of the common violations to the last one, and add ._ to the autogenerated nwbfile_path of the last one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Closer - it will have to be an additional one tacked onto the end though, not index 2 (since that already exists and has specific tests for it)

Either that or a simpler standalone unit test for just this particular behavior

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok now the sidecar test file is the last one in the list and the no-error file is back to being the third

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.05%. Comparing base (4fa24bf) to head (91c1fcd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #470      +/-   ##
==========================================
+ Coverage   87.04%   87.05%   +0.01%     
==========================================
  Files          23       23              
  Lines        1250     1251       +1     
==========================================
+ Hits         1088     1089       +1     
  Misses        162      162              
Flag Coverage Δ
unittests 87.05% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/nwbinspector/nwbinspector.py 85.71% <100.00%> (+0.05%) ⬆️

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) May 30, 2024 16:18
@CodyCBakerPhD CodyCBakerPhD merged commit 9f242ff into NeurodataWithoutBorders:dev May 30, 2024
37 of 38 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the sidecar branch May 30, 2024 16:18
@CodyCBakerPhD
Copy link
Contributor

Thanks @pauladkisson !

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.

[Bug]: nwbinspector fails on folders located on external ssd for mac due to ._ sidecar files
3 participants