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

Fix/python deps #189

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix/python deps #189

wants to merge 6 commits into from

Conversation

tomalrussell
Copy link
Member

Bumping dependency versions in line with nismod/irv-jamaica#204

@tomalrussell tomalrussell marked this pull request as ready for review November 20, 2024 11:35
@@ -43,18 +43,23 @@ async def get_colormap(

::kwarg num_values int Number of values to generate in the colormap
"""
if stretch_range is None:
stretch_range_arg: list[int] = [0, 10]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for this particular default value? I am wondering if it's better to have a default here, or just require a parameter and explicitly fail when it's not supplied.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a quick guess based on the datasets I'm familiar with! Should we fail with a 400 bad request?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some 4xx error for sure - doesn't FastAPI already return the appropriate error if we remove Optional from the parameter and it's missing from the request?
I'd say that not having a default sounds safer, just because the value is purely dependent on the data domain so in most cases it should be explicitly provided anyway - and presumably if some client code was forming requests incorrectly (e.g. with a typo in the stretch_range parameter name), this could lead to confusing behaviour where the default would be used and the tileserver would return tiles with incorrect colours but that would be difficult to spot.


@validator("eael_days")
@field_validator("eael_days")
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're at it - was the reason for keeping this quirk fix since this was first released because it was too time consuming to recalculate the raw data?

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