feat(python): expand layer management and headless camera API - #1770
Merged
Conversation
Scripting a map from Python needed a browser round trip or a raw project dict for common tasks: reordering, duplicating, renaming layers, reading attribute values, and moving the camera. These now work as plain project mutations on Map and Layer, and the project authoring helpers are exported from the top-level package for scripts that never display a widget.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Python API adds layer management and inspection helpers, persisted camera and project metadata APIs, top-level authoring exports, documentation, and tests for headless project operations. ChangesPython authoring API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Map
participant AuthoringResolver
participant Layer
Caller->>Map: Resolve layer reference
Map->>AuthoringResolver: Match ID or name
AuthoringResolver-->>Map: Return layer record
Map->>Layer: Apply layer operation
Layer-->>Caller: Return updated state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/README.md`:
- Around line 99-106: Create the “Roads” layer before calling
find_layer("Roads") in the README example, or explicitly document that an
existing layer is required, ensuring the snippet does not dereference None when
no such layer exists.
In `@python/src/geolibre/__init__.py`:
- Around line 16-27: Reorder the exports in __all__ so "__version__" appears
before the lowercase names, satisfying Ruff RUF022 while preserving all existing
exports.
In `@python/src/geolibre/geolibre.py`:
- Around line 2469-2471: Update Layer.index to validate the handle by calling
self._layer() before searching the map layers, ensuring removed layers raise the
same documented ValueError as other Layer accessors instead of StopIteration.
- Around line 1000-1005: Update duplicate_layer to validate the requested name
through the same reserved-name validation path used by rename_layer, before
calling _add_layer; ensure name="__basemap__" is rejected while existing
default-name behavior remains unchanged, and add a regression test covering this
reserved name.
- Around line 979-1025: Update Map._resolve_layer and the removal path around
python/src/geolibre/geolibre.py:1959-1960 to resolve string references through
_authoring.find_layer before constructing or using a Layer handle, preserving
case-insensitive matching and duplicate-name rejection for all mutations and
removals. Add coverage in python/tests/test_scripting.py:627-655 for duplicate
display names and case-insensitive references.
- Around line 2017-2023: Update set_center in python/src/geolibre/geolibre.py
around lines 2017-2023 to route map updates through _authoring.set_view instead
of writing mapView directly, ensuring a manual recenter clears any persisted
bbox. Add or update the test in python/tests/test_scripting.py around lines
692-695 to fit project bounds, call set_center, and assert mapView no longer
contains bbox.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d3080458-dde6-4daf-ae57-c3381c6502cf
📒 Files selected for processing (5)
python/README.mdpython/src/geolibre/__init__.pypython/src/geolibre/geolibre.pypython/tests/test_project.pypython/tests/test_scripting.py
Contributor
🔍 Cloudflare PR preview
|
Contributor
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
I checked the resolve/mutate plumbing ( |
- Route Map._resolve_layer through authoring.find_layer so scripting and the MCP tools agree on what a layer reference means: id first, then exact name, then case-insensitive, with a duplicated name raising instead of silently picking one. Now that remove_layer accepts names, an arbitrary pick would delete the wrong layer. - Validate duplicate_layer's explicit name against the reserved basemap pseudo-id, which rename_layer already rejects via update_layer. - Route set_center through authoring.set_view so a manual recenter clears the bbox fit_project_bounds recorded, instead of leaving a stale extent behind. - Make Layer.index raise the documented ValueError for a removed handle rather than a bare StopIteration, matching the other accessors. - Add Map.bearing and Map.pitch read properties so every camera setter has a matching getter. - Correct the move_layer docstring: negative values follow sequence indexing, not list.insert semantics. - Sort __version__ into __all__ (RUF022) and make the README layer-handle example self-contained.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/README.md (1)
92-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
bearingandpitchproperties.The persisted-state property list omits
bearingandpitch. Include both getters so users can discover how to read persisted camera orientation.Proposed fix
-| `center` / `zoom` / `basemap` / `name` | Read persisted project and camera state; `name` is writable. | +| `center` / `zoom` / `bearing` / `pitch` / `basemap` / `name` | Read persisted project and camera state; `name` is writable. |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/README.md` at line 92, Update the persisted-state property list in the README to include the read-only bearing and pitch getters alongside center, zoom, basemap, and name, while preserving the existing note that only name is writable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/tests/test_scripting.py`:
- Around line 676-700: Resolve the Ruff violations in the tests: inside the
pytest.raises block around duplicate.index, assign the property result to _
while preserving the expected ValueError. In
test_duplicate_layer_rejects_the_reserved_basemap_name, constrain pytest.raises
with a match for the reserved-name error message instead of accepting any
ValueError.
---
Outside diff comments:
In `@python/README.md`:
- Line 92: Update the persisted-state property list in the README to include the
read-only bearing and pitch getters alongside center, zoom, basemap, and name,
while preserving the existing note that only name is writable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 201e895d-9a8b-4108-a7ac-0d7d1fb7051e
📒 Files selected for processing (4)
python/README.mdpython/src/geolibre/__init__.pypython/src/geolibre/geolibre.pypython/tests/test_scripting.py
- Bind the bare `duplicate.index` access to `_` so the statement reads as a deliberate property evaluation rather than dead code. - Match on the reserved-name message instead of accepting any ValueError, so the test cannot pass on an unrelated failure.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
I also verified that the refactored |
- Route duplicate_layer through authoring.add_layer instead of reaching into the private _reject_reserved_name, and reject a blank explicit name rather than silently substituting the auto-generated one, matching the Map.name setter. - Stop deep-copying the whole project in describe(): copy the small summary instead, which detaches the live mapView it returns without duplicating every inlined GeoJSON blob to report a feature count. - Document that remove_layer now raises on an unresolved or ambiguous reference, where it used to be a silent no-op. - Cover move_layer's negative-index semantics, which differ from list.insert(-1, ...) and are easy to reintroduce a bug in.
Contributor
|
This all checks out. I've done a thorough trace of the new methods, verified the negative-index math in Code reviewBugs: None found. Traced Security: None found. All new methods operate on in-memory dicts; no new file I/O, network calls, or string interpolation into commands/queries. Performance: None found. Quality:
CLAUDE.md: No violations found. The new |
- Strip an explicit duplicate_layer name before storing it, matching the Map.name setter, so " Clone " does not become a padded name that is awkward to reference back. - Document that the copy is appended to the draw order rather than placed next to its source, which was previously unstated either way.
Member
Author
|
Addressing the three quality nits from the latest review summary (they were not attached to lines, so replying here):
|
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Posted 4 inline comments (1 medium-confidence security note, 1 medium-confidence validation-inconsistency bug, 2 low-confidence documentation/naming nits). |
- Sweep credentials from Layer.source and Layer.data. Both hand a layer record straight to a caller, and a notebook auto-displays whatever a cell returns, so a source built with request_headers or a signed URL would print its secrets into an output that often gets committed. Factored the per-layer sweep redact_credentials already ran into project.redact_layer so the two paths cannot drift. - Reject a blank name in rename_layer (and so in the Layer.name setter, which delegates to it); authoring.update_layer guards only the reserved basemap pseudo-id. Shares one _clean_layer_name helper with duplicate_layer. - List the bearing and pitch read properties in the README table.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Redact Map.basemap the way Layer.source is redacted. MapTiler and Stadia put an API key in the style URL itself, so reading it back in a notebook printed the key; project.redact_url is the public entry point for the sweep redact_credentials already applied to that field. - Document that Layer.data copies an inlined geojson blob whole, and point at properties()/describe() for the cases a summary covers.
Contributor
Code reviewSecurity
Bugs
Performance
Quality
CLAUDE.md
|
Redact URLs in layer_summary and describe_project rather than at the Map.describe call site. A summary exists to be shown, and both callers show it somewhere untrusted: a notebook cell that often gets committed, and an MCP tool result that goes to a model client. Fixing it in authoring.py closes the same gap in the MCP server's describe_project tool and keeps the one-place-per-change layering the repo documents.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Map(rename_layer,move_layer,duplicate_layer,show_layer,hide_layer) and letremove_layeraccept an id, display name, orLayerhandle like the other layer methods.layer_properties,column_values,describe, plusLayer.source/.data/.index/.properties()/.column()/.move()/.duplicate().set_zoom,set_bearing,set_pitch,fit_project_bounds, andcenter/zoom/bearing/pitch/basemap/nameproperties), and export the headless authoring helpers (load_project,save_project,describe_project,basemap_catalog,builtin_legend_names,color_ramp_names) from the top-levelgeolibrepackage.Behavior changes worth a reviewer's attention
Map._resolve_layernow delegates toauthoring.find_layer, so every method that takes a layer reference (including the pre-existingset_layer_visibility,set_layer_opacity,zoom_to_layer) resolves an id first, then an exact name, then a case-insensitive one. A name that several layers share now raises instead of silently picking the first match. This makes the scripting API and the MCP tools agree on what a reference means.Map.find_layerkeeps its first-match /Nonecontract for leafmap compatibility.remove_layeron an unknown reference now raisesValueErrorwhere it used to be a silent no-op. A caller that wants the old semantics can guard withif m.find_layer(name):.Map.basemap,Layer.source, andLayer.dataredact credentials, matching whatto_project()andsave_project()already did. A notebook auto-displays whatever a cell returns, so an API key in a style URL or arequest_headersblob would otherwise print into an output that often gets committed.Map.projectstill returns everything exactly as stored.set_centernow routes throughauthoring.set_view, so recentering clears thebboxa previousfit_project_boundsrecorded rather than leaving it describing a stale extent.Test plan
cd python && pytest(257 passed, 3 skipped)pre-commit run --files <changed files>clean, including the npm build hooksave_projectand reopen it