WIP: Pulling layers from ArcGIS from within Windmill anonymously #154
Conversation
|
Actually @nicopace it appears you are not done yet, right? I'll move this back to "In Progress" and wait to hear from you when you want to me to look at it. |
There was a problem hiding this comment.
This first review is mostly intended as an orientation to get your code in-line with conventions across the repo, and clarifying why your tests are failing. I have otherwise not done a thorough, line-by-line code review.
A few additional things (I know this is still WIP, but just for purposes of comprehensive orientation):
- Add this new script to
tox.inito test using tox (see "Running Tests" in the main README). I think you may have been using pytest directly and that is now how testing is set up in this repo. I also recommend you look at the test code for some of the other scripts, most salient of all the existingarcgis_feature_layerscript. - You need an
arcgis_download_feature_anonymous.script.yamlfile to pass the params via Windmill. Please have a look at some other script yaml files in the repo to get a sense of how this works. - The
script.lockfile corresponding to each script is generated by Windmill when run. You can sync pull it and then commit it to your branch. I can show you how to set up the Windmill CLI if you haven't already. - Please also describe this script in the
f/arcgis/README.
f/connectors/arcgis/arcgis__download_feature_layer_anonymously.py
Outdated
Show resolved
Hide resolved
f/connectors/arcgis/arcgis__download_feature_layer_anonymously.py
Outdated
Show resolved
Hide resolved
f/connectors/arcgis/arcgis__download_feature_layer_anonymously.py
Outdated
Show resolved
Hide resolved
f/connectors/arcgis/arcgis_download_feature_layer_anonymously.py
Outdated
Show resolved
Hide resolved
f/connectors/arcgis/arcgis_download_feature_layer_anonymously.py
Outdated
Show resolved
Hide resolved
|
|
||
| # Configure module logger | ||
| logger = logging.getLogger(__name__) | ||
| logger.addHandler(logging.NullHandler()) |
There was a problem hiding this comment.
Preventing unwanted log output from libraries: When developing a library, it is considered good practice to add a NullHandler to the library's loggers. This prevents the library's log messages from being propagated to the root logger of an application that uses the library, and thus prevents them from appearing in the application's logs unless the application explicitly configures a handler for that specific library's logger. This helps keep the application's logs clean and focused on application-specific events.
Basically, logs will not propagate when importing the library, unless you explicitly want to log those as user of the library.
f/connectors/arcgis/tests/arcgis__download_feature_layer_anonymously_test.py
Outdated
Show resolved
Hide resolved
| def _create_test_session(monkeypatch, metadata_resp: Dict[str, Any], query_resp: Dict[str, Any]): | ||
| """Helper to create a fake session by monkeypatching requests.Session.get""" | ||
| class DummyResp: | ||
| def __init__(self, data, status=200): | ||
| self._data = data | ||
| self.status_code = status | ||
|
|
||
| def raise_for_status(self): | ||
| if self.status_code >= 400: | ||
| raise requests.HTTPError(f"{self.status_code} error") | ||
|
|
||
| def json(self): | ||
| return self._data | ||
|
|
||
| def iter_content(self, chunk_size=8192): | ||
| yield b"abc" | ||
|
|
||
| class DummySession: | ||
| def __init__(self): | ||
| self.calls = [] | ||
|
|
||
| def get(self, url, params=None, **kwargs): | ||
| self.calls.append((url, params)) | ||
| if url.endswith("?f=pjson"): | ||
| return DummyResp(metadata_resp) | ||
| if "/query" in url: | ||
| return DummyResp(query_resp) | ||
| # attachments info or download | ||
| if url.endswith("/attachments"): | ||
| return DummyResp({"attachmentInfos": []}) | ||
| if "/attachments/" in url: | ||
| return DummyResp(b"binarycontent") | ||
| return DummyResp({}, status=404) | ||
|
|
||
| return DummySession() |
There was a problem hiding this comment.
I don't think you need to do this. Please see conftest.py in this directory to see how we are already mocking an ArcGIS server.
There was a problem hiding this comment.
The tests are already passing with this code.
Will finish with the rest of the reviews first.
There was a problem hiding this comment.
The tests may be passing, but it would be good to use the same patterns as elsewhere in this repository, to improve maintainability and developer experience. If every script (and its tests) has a somewhat different way of doing things, then it becomes harder to make changes across the board.
However, if you'd like, I can take it from here and bring your script in-line with repo-wide patterns. (And also make an effort to make those more explicit.)
|
Gone through all your comments. Agree with them. Will do changes and update this branch, then will quote you again for review. Thanks |
|
@nicopace maybe one more thing to add is information in the README on where to get the params. In particular, it could be tricky to find the layer ids. |
2 participants
Adds a script to fetch layers from ArcGIS Feature Services anonymously, based on a previously written script.
TODO: