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

Default to "None" for the limit #1359

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Conversation

psavery
Copy link
Collaborator

@psavery psavery commented Nov 2, 2023

On the master branch, if I try to import with no limit set, I get the following error:

Invalid value for integer parameter limit: .

We should remove the datatype restriction and set the default to None. That way, it will work properly.

@manthey
Copy link
Member

manthey commented Nov 2, 2023

I think with this change, we have to change line https://github.com/girder/large_image/blame/master/sources/dicom/test_dicom/web_client_specs/dicomWebSpec.js#L115 back to return $('.g-validation-failed-message').html() === 'Invalid limit'; for the tests to pass.

On the master branch, if I try to import with no limit set, I get the
following error:

```
Invalid value for integer parameter limit: .
```

We should remove the datatype restriction and set the default to `None`.
That way, it will work properly.

Signed-off-by: Patrick Avery <[email protected]>
@psavery psavery force-pushed the limit-default-none branch from 42735f2 to 2a2d355 Compare November 2, 2023 18:53
@psavery
Copy link
Collaborator Author

psavery commented Nov 3, 2023

I think with this change, we have to change line https://github.com/girder/large_image/blame/master/sources/dicom/test_dicom/web_client_specs/dicomWebSpec.js#L115 back to return $('.g-validation-failed-message').html() === 'Invalid limit'; for the tests to pass.

Done. Looks like the CI is passing now.

@manthey manthey merged commit 295a894 into girder:master Nov 3, 2023
5 checks passed
@psavery psavery deleted the limit-default-none branch November 14, 2023 23:05
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