Additional get params

[chinacat567]

Context

• Issue discussed here Support custom GET parameters when retrieving images #105

Change

Passing additional GET params as a query string to the following methods

GET Methods

Retrieve functions

Data

• retrieve_study
• retrieve_instance
• retrieve_series
• retrieve_instance_frames
• retrieve_bulkdata

NOTE : retrieve_instance_rendered and retrieve_instance_frames_rendered contain a preexisitng arugment 'params', hence no modifications were made to these

Metadata

• retrieve_study_metadata
• retrieve_series_metadata
• retrieve_instance_metadata

Iter functions

• iter_study
• iter_series
• iter_bulkdata

Search functions

• search_for_instances
• search_for_studies
• search_for_series

POST Methods

• store_instances

DELETE Methods

• delete_study
• delete_instances
• delete_series

Testing

• Added tests to check that that the additional params are being passed as a query string to the request for each method
• pytest tests/test_web.py
• pytest --flake8

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[CPBridge]

Hi @chinacat567

Thanks for this pull request, but there appear to be many changes unrelated to the purpose of the PR. For example pyproject.toml and setup.py have changed for no apparent reason, and many files have been renamed. Could you please undo those changes so that the PR is limited just to the addition of additional http parameters?

[CPBridge]

Please remove unnecessary changes

[chinacat567]

Hi @chinacat567

Thanks for this pull request, but there appear to be many changes unrelated to the purpose of the PR. For example pyproject.toml and setup.py have changed for no apparent reason, and many files have been renamed. Could you please undo those changes so that the PR is limited just to the addition of additional http parameters?

Sorry about that! Those commits were accidental — I’ll undo them soon.

[CPBridge]

One very minor change. Otherwise I think this looks good to me, thank you @chinacat567 .

@pieper any other thoughts?

[medihack]

It makes sense to add that additional_params parameter to the store functions. We don't have a direct use for it, but for consistency.

@CPBridge By the way, in our fork we switched over to using uv with an automatic publish workflow and dynamic versioning. So if you are interested, we can contribute it back to this repo. I wasn't sure how well dicomweb-client is currently maintained, and I have no problem with deleting our fork again.

[CPBridge]

I have approved. We'll give a few days to see if @pieper has a different opinion, and then we can merge and I'll push out a 0.60.0 release.

@CPBridge By the way, in our fork we switched over to using uv with an automatic publish workflow and dynamic versioning. So if you are interested, we can contribute it back to this repo.

@medihack I think switching to improved tooling would be a good thing if you are happy to contribute!

I wasn't sure how well dicomweb-client is currently maintained, and I have no problem with deleting our fork again.

I am committed to maintaining this repo going forward following @hackermd stepping back (though there was a bit of a dead period during the transition), but it isn't my highest priority and I don't plan to add any major new features myself, though I'm happy to consider PRs. Do you have any particular changes you might be looking to make in the future?

[medihack]

I am committed to maintaining this repo going forward following @hackermd stepping back (though there was a bit of a dead period during the transition), but it isn't my highest priority and I don't plan to add any major new features myself, though I'm happy to consider PRs. Do you have any particular changes you might be looking to make in the future?

@CPBridge Fair enough, but I also can't think of any major new features (maybe additional async support by using httpx, but we currently don't need that with ADIT, in which we use this library).

How do you currently release new versions to PyPI? The nice thing about the GitHub Action publish workflow we added to our fork is that it will automatically get published to PyPI with the same version when you make a GitHub release. But not sure if you want something like that.

[pieper]

Thanks for the contribution 👍 Like @CPBridge I want to see this project continue but it's not a high priority. I also would like to see async support since it's important for interactive use, but it's also not high on my priority list.

Regarding this PR, should the opional headers be Dict[str, str] and not Dict[str, Any]?

[chinacat567]

Regarding this PR, should the opional headers be Dict[str, str] and not Dict[str, Any]?

Using Dict[str, Any] to pass parameters is a consistent pattern already established in retrieve_series_rendered and retrieve_instance_rendered. A sample test case for this involves nested query parameters represented as key-value mappings. If a key needs to appear multiple times with different values, the values should be provided as an iterable—for example, {"key": ["value1", "value2"]}

[pieper]

If a key needs to appear multiple times with different values, the values should be provided as an iterable—for example, {"key": ["value1", "value2"]}

Oh, interesting, I didn't realize that was an option! I guess that's the point of the typing annotations so these things can be made more clear. It doesn't need to be part of this PR, but perhaps we should refine these typings to be more specific that the values can be strings or lists of strings (anything else?) rather than just Any.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[chinacat567]

@CPBridge By the way, in our fork we switched over to using uv with an automatic publish workflow and dynamic versioning. So if you are interested, we can contribute it back to this repo.

@medihack I think switching to improved tooling would be a good thing if you are happy to contribute!

@CPBridge I have created a PR for this here

[CPBridge]

Thanks @chinacat567 !

[CPBridge]

P.s. I'll maybe wait for #114 before making a new release. Unless you think that doesn't make sense?