Skip to content

Conversation

@WillForan
Copy link

also address issue #27 and could supersede pull request #28

I pulled out themeta length check into it's own function and added a test. I also moved the narrow_participants call before temporary files are copied.

The test is only checking narrowing works, not any down stream effects. And I'm worried I've likely overlooked some important considerations. Please feel free to ruthlessly reject

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 6, 2023

will try to review in the coming days

makes me think that this kind of thing should probably be delegated to bids-matlab which is an update and better version of spm_bids

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 6, 2023

but already thanks for adding tests!!!

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 6, 2023

Note to self:

  • circle CI should run on PR
  • add code linting
  • use matlab github actions to run tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think you could rewrite this test to be used directly by matlab testing framework?

Ideally with setup and teardown functions.

https://nl.mathworks.com/help/matlab/matlab_prog/write-test-using-setup-and-teardown-functions.html

If you don't have the bandwidth, let me know and I can give it a go

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 8, 2023

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Nov 8, 2023

Seems that we have a path issue when testing it:

https://app.circleci.com/pipelines/github/bids-apps/SPM/44/workflows/dd6d3670-b56f-419b-bb88-b8b6ec8f8021/jobs/220

that's because the new m file you have added is not copied into the container:

SPM/Dockerfile

Line 41 in 68e5b93

COPY run.sh spm_BIDS_App.m pipeline_participant.m pipeline_group.m /opt/spm${SPM_VERSION}/

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.

2 participants