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

FIX: Resolve side-effects of new testfile in #682 #695

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

oesteban
Copy link
Collaborator

Should take away the problem identified in #693 (comment), but without removing the test.

@oesteban
Copy link
Collaborator Author

oesteban commented Jan 12, 2021

The packaging test is failing, some recent changes on Pypi must've broken it. cc/ @effigies

@tyarkoni
Copy link
Collaborator

I appreciate the effort, and it's true that this fixes the current test failures, but it doesn't actually solve the underlying problem identified in #694, which is that so long as entity values can be collections, certain parts of the codebase may choke unexpectedly. So I think we should actually leave the offending test file in, and then figure out how to deal with it in a principled way (hence #694). WDYT?

@effigies
Copy link
Collaborator

The package test depends on a secret, which isn't available to forks. I should update the test not to run on PRs.

@oesteban
Copy link
Collaborator Author

I think this PR rolls back the test data to its original state (which is also preferred as we are not clear this particular bending of the metadata is actually permissible or should be allowed) while showing one example of how to write tests that generate edge cases on a temporary directory. At least from an educational point of view for future contributors, this could be useful.

I agree it doesn't really add much otherwise, although, in my settings and for my needs, PyBIDS is currently fairing well so I don't feel the urgency of #694. -- but that's my use-case.

Anyways, I guess that changes such as #694 will likely come with (at least) a minor release number bump - from a releasing point of view, adding this minimal change is safer than not doing it (because it is preempting others to hit problems with the incorrect metadata definition), while it shows how to build more tests of this particular issue.

Since I do not consider myself a maintainer of PyBIDS, I cannot tell if I'll be able to keep an eye on this particular family of issues and I'd understand that you decided against these changes from the maintenance-burden point of view (although I tried to change that view). No biggie there either.

@effigies
Copy link
Collaborator

It is currently hard to evaluate other PRs because of the broken tests. I'm a bit inclined to resolve this and then create an open PR reverting it where the principled solutions discussed in #694 can be developed.

Does that work for both of you, @oesteban @tyarkoni?

@tyarkoni
Copy link
Collaborator

Fine with me!

@effigies effigies merged commit 5747a18 into bids-standard:master Jan 21, 2021
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.

3 participants