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

Use a more complex chunks header #7

Merged
merged 9 commits into from
Feb 2, 2023
Merged

Conversation

andersy005
Copy link
Member

This PR is an attempt at implementing a more complex chunks header as suggested by @rabernat in #6 (comment).

with this PR, we should be to define headers as follows:

'chunks: lat=10,air=10,10'

on FastAPI side, this is parsed into a a dictionary

In [1]: from zarr_proxy.logic import parse_chunks_header

In [2]: parse_chunks_header('lat=10,air=10,10')
Out[2]: {'lat': (10,), 'air': (10, 10)}

Cc @katamartin

@andersy005
Copy link
Member Author

@rabernat, do you have any thoughts/feedback on this? :)

@rabernat
Copy link
Member

My main feedback is that your should write down somewhere (maybe just the main README?) what exactly the specification is for the header, and what will happen for missing and or non-conforming headers.

I would also write down how clients are supposed to figure this out.

My concern is that the implementation here is very tightly coupled to your front-end development needs. That's fine, but just make sure you document the interface clearly.

@andersy005
Copy link
Member Author

thank you, @rabernat!

I would also write down how clients are supposed to figure this out.

i plan to add docs in the coming days and will certainly make sure this is covered.

My concern is that the implementation here is very tightly coupled to your front-end development needs. That's fine, but just make sure you document the interface clearly.

👍🏽

@andersy005 andersy005 merged commit 38507bd into main Feb 2, 2023
@andersy005 andersy005 deleted the headers-improvements branch February 2, 2023 00:13
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