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

feat: add query parameter for allowing datasets #429

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

SimonThordal
Copy link
Contributor

Closes #427

@SimonThordal SimonThordal force-pushed the filter-datasets-by-query-param branch from c3d1e31 to fa8c8a2 Compare April 15, 2024 09:45
Copy link
Contributor

@jbothma jbothma left a comment

Choose a reason for hiding this comment

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

Nice!

tests/unit/test_match.py Outdated Show resolved Hide resolved
@pudo
Copy link
Member

pudo commented Apr 15, 2024

Nitpick: we have exclude_dataset and now we're adding allow_datasets (plural). What if we tried to make these matching, e.g. include_dataset for the new one?

@SimonThordal
Copy link
Contributor Author

Nitpick: we have exclude_dataset and now we're adding allow_datasets (plural). What if we tried to make these matching, e.g. include_dataset for the new one?

That was my first choice actually, but I changed it since the path parameter already is called dataset and I thought it made it clearer that we were further refining that first dataset / collection. I'm good with either though. As for plural it's for making it clear that it's a list and not a single string.

@pudo
Copy link
Member

pudo commented Apr 15, 2024

Both of them are lists - only the dataset is part of the path, right?

@SimonThordal
Copy link
Contributor Author

Both of them are lists - only the dataset is part of the path, right?
True, that was part of my confusion at first. From the signature of /match it sounded as though I should specify a single dataset as the path parameter dataset and then I could pass a single dataset as exclude_dataset.

It becomes clear once you read through the code of course, but I think something like {collection}/match?exclude_datasets=[]&include_datasets=[] would have been easier to understand at first glance.
I'll switch back to include_dataset for now though and then we can always see whether we want to change this later :)

@SimonThordal SimonThordal force-pushed the filter-datasets-by-query-param branch from fa8c8a2 to b7885b2 Compare April 16, 2024 06:42
@SimonThordal SimonThordal marked this pull request as draft April 16, 2024 07:20
@SimonThordal
Copy link
Contributor Author

Just saw that we want to add this to /search as well, so I'll include that here.

@SimonThordal SimonThordal force-pushed the filter-datasets-by-query-param branch from b7885b2 to 538b0a0 Compare April 16, 2024 08:46
@SimonThordal SimonThordal marked this pull request as ready for review April 16, 2024 08:46
@SimonThordal SimonThordal requested a review from pudo April 16, 2024 08:46
@SimonThordal SimonThordal merged commit 1c5645b into main Apr 16, 2024
4 checks passed
@SimonThordal SimonThordal deleted the filter-datasets-by-query-param branch April 16, 2024 11:48
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.

Support for dataset allow-listing in /search and /match APIs
3 participants