Conversation
eveleighoj
left a comment
eveleighoj
left a comment
There was a problem hiding this comment.
A few things to fix and a different approach for validation but I think it'll be worth it.
Worth reviewing the testing guidance here https://digital-land.github.io/technical-documentation/development/testing-guidance/
if there is stuff I've mentioned that isn't in there please add it in! the repo is here:
| except Exception as exception: | ||
| logging.warning(exception) | ||
| log["exception"] = type(exception).__name__ | ||
| log["arcgis-step"] = "feature-iteration" |
There was a problem hiding this comment.
let's not alter the data in the log. the log is generic across all getters adding arcgis data in will confuse them.
This goes for further additions below as well
| @@ -0,0 +1,40 @@ | |||
| def positive_int(value, plugin_name, parameter_name): | |||
There was a problem hiding this comment.
remove this and use pydantic dataclasses for validation, this removes the need for boilerplate validation. code and is easily expandable for other plugins moving forward.
dataclasses even offer validation for the parameters column in the future which I'm sure will come up.
you could even use the factory pattern instead of the if/else block in the Colector.fetch method.
you can just create a dataclass in the plugin file
There was a problem hiding this comment.
This has now been refactored into a dataclass with validation in it.
I have moved the validation into collect.py as discussed (params are validated when they are passed into get func) and made the validation generic for other plugins to use.
| assert log["exception"] == "TypeError" | ||
|
|
||
|
|
||
| def test_arcgis_get_returns_no_partial_string_on_iteration_failure(monkeypatch): |
There was a problem hiding this comment.
tests against. the arcgis_get function should. be in tests/unit/plugins/test_arcgis.py
| assert log["exception"] == "TypeError" | ||
|
|
||
|
|
||
| def test_arcgis_get_returns_no_partial_string_on_iteration_failure(monkeypatch): |
There was a problem hiding this comment.
use pytest-mocker though the mocker fixture instead. it's already in the codebase and we should standardise mocking approaches
eveleighoj
left a comment
eveleighoj
left a comment
There was a problem hiding this comment.
Mainly just a question around the expected behaviour and checking in with the product manager and data manager on how we want it to error under validation.
My thoughts are that if incorrect parameters are supplied then it should error but not exit the process.
Also you will need to rebase your changes on main as another PR has gone in
|
|
||
| content += "]}" | ||
| last_exception = None | ||
| for attempt in range(1, retries + 2): |
There was a problem hiding this comment.
retries is being used twice, both in the dumper and in our code through manual looping, do we need both?
| return FetchStatus.EXPIRED, None | ||
|
|
||
| if parameters is None: | ||
| parameters = {} |
There was a problem hiding this comment.
do we need to define the parameters as an empty dict here? can we just only validate if parameters aren't none otherwise pass none to the get function?
| try: | ||
| parameters = json.loads(raw_parameters) | ||
| except json.JSONDecodeError as exc: | ||
| raise ValueError( |
There was a problem hiding this comment.
linked to the above but we want to think about what errors are being raised, a validation error that pydantic raises is more descriptive than a value error
| from digital_land.plugins.arcgis import get as arcgis_get | ||
|
|
||
|
|
||
| def test_arcgis_get_returns_partial_string_on_iteration_failure(mocker, caplog): |
There was a problem hiding this comment.
tiny detasil but the function is called get not arcgis get, should be reflected in the tests
| assert log["exception"] == "TypeError" | ||
|
|
||
|
|
||
| def test_fetch_passes_parameters_to_arcgis_plugin(collector, mocker): |
There was a problem hiding this comment.
as it's testing a method it should be inside a TestCollector class
2 participants
What type of PR is this? (check all applicable)
Description
This PR improves ArcGIS fetch robustness by supporting configurable timeout, retries, and retry_backoff_seconds, and by logging which ArcGIS step failed thereby getting more successes in downloading arcgis data.
This PR also tightens and generalises plugin parameter handling around ArcGIS collection.
It extracts parameter validation into a shared utility at digital_land/utils/validate_parameter_utils.py, so plugins can define defaults and validation rules without duplicating validation code.
Tests were extended in tests/unit/test_collect.py to cover reusable validation behaviour, rejection of unknown parameters, ArcGIS default parameter expansion, and retry/failure logging.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above. Please refer to the Digital Land Testing Guidance for more information.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?