gacou54 / pyorthanc

Python library that wrap the Orthanc REST API and facilitate the manipulation of data in Orthanc
MIT License
49 stars 10 forks source link

Headers parameter in find #16

Closed jrkerns closed 1 year ago

jrkerns commented 1 year ago

Hey, great project!

Could you add a headers parameter to the find function (https://github.com/gacou54/pyorthanc/blob/b41a7eee32295f8b11942d5445366679bd1000bc/pyorthanc/filtering.py#L30). Headers are allowed in the Orthanc instantiation. We use Orthanc behind a proxy and requires a header. I'd love to use find via the proxy.

gacou54 commented 1 year ago

Hi @jrkerns,

Absolutely, that would be a great addition. I am thinking to pass an Orthanc object instead of the server information.

Something like that:

def find(orthanc: Orthanc, patient_filter, ....):
jrkerns commented 1 year ago

Well, it's your lib so you can do what you want, but in my experience top-level functions usually take primitives only in their parameters. E.g. the user will always have to do:

orthanc = Orthanc(...)
results = find(orthanc, ...)

which seems wordy, but it's mostly an aesthetic problem to me.

Another approach would be to simply make find a method of the Orthanc instance. So

orthanc = Orthanc(...)
orthanc.find(...)

That's my favorite solution, but know I have a bias for classes 😄

I see you have async options as well. In theory, you could keep the best of both worlds and still have a top level function per your suggestion and then use that in your method. Assuming the solution you proposed you can also do:

from pyorthanc import find

class Orthanc:

     def find(self, ....):
           return find(self, ...)

class AsyncOrthanc:

    def find(self, ...):
          yield find(self, ...)

I'm not an async guy so forgive the async simplification.

gacou54 commented 1 year ago

Fix in #17 . Added to the lasted release PyOrthanc 1.11.5.