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

Code changes to make this repo compatible with datalad-catalog>=1.1.0 #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsheunis
Copy link
Contributor

Closes #43

@jsheunis
Copy link
Contributor Author

jsheunis commented Jan 10, 2024

The appveyor run fails because it still installs the older version of datalad-catalog via the singularity image: https://ci.appveyor.com/project/mih/inm-icf-utilities/builds/48919129#L475. The PR branch is not checked out by this code, therefore it will always take the requirements-devel.txt file state from the main branch of the remote at https://github.com/psychoinformatics-de/inm-icf-utilities, which means testing for dependency updates cannot occur usefully via this standard PR testing route.

Same problem noted here: #42 (comment)

Copy link
Collaborator

@mslw mslw left a comment

Choose a reason for hiding this comment

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

Bundling an updated catalog would have several advantages, incl. fixing the caching issues (datalad/datalad-catalog#457) recently reported by e-mail by @ste-phi

@jsheunis it's been a while but do you think it's good to go?

It builds a catalog for me when tested on some of my local data, so for me that's a yes.

Comment on lines +92 to +95
catalog_add(
catalog=catalog_path,
metadata=study_entry,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe - just maybe - this could have default tab set to subdatasets (the study dataset has only subdatasets, no content) but IIRC catalog_add can take only a config file, not an individual option, so I'm not inclined to change it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but IIRC catalog_add can take only a config file, not an individual option

That's correct. There was an intention to update the code of catalog_set to allow the config option, which would reset the config. But this hasn't been implemented.

@@ -3,5 +3,5 @@ datalad-next
pydicom
pytest
pytest-env
datalad-catalog==0.2.1b0 --pre
datalad-catalog
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would make sense to still pin the version, just to the current one, to avoid having to deal with future changes? Or is that unnecessary?

Suggested change
datalad-catalog
datalad-catalog==1.1.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ideally we wouldn't pin it, but under the circumstances of datalad-catalog receiving breaking changes sometimes, and the future development being unclear at the moment, I would agree with pinning it here.

@jsheunis
Copy link
Contributor Author

Looks good. I reran the PR appveyor job and all are successful now.

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.

Tests do not work with the latest datalad-catalog release
2 participants