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

Add remove_layer method to Python GISDocument API #478

Merged
merged 6 commits into from
Feb 21, 2025

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Feb 19, 2025

Description

@YaoTingYao and I pair programmed on this during the hackathon!

Resolves #439

Checklist

  • PR has a descriptive title and content.
  • PR description contains references to any issues the PR resolves, e.g. Resolves #XXX.
  • PR has one of the labels: documentation, bug, enhancement, feature, maintenance
  • Checks are passing.
    Failing lint checks can be resolved with:
    • pre-commit run --all-files
    • jlpm run lint

📚 Documentation preview: https://jupytergis--478.org.readthedocs.build/en/478/
💡 JupyterLite preview: https://jupytergis--478.org.readthedocs.build/en/478/lite

@mfisher87 mfisher87 added the enhancement New feature or request label Feb 19, 2025
Copy link
Contributor

Binder 👈 Launch a Binder on branch mfisher87/jupytergis/python-api-remove-layer

Copy link
Contributor

github-actions bot commented Feb 19, 2025

Integration tests report: appsharing.space

@mfisher87 mfisher87 changed the title Add remove_layer method to Python document API Add remove_layer method to Python GISDocument API Feb 19, 2025
@mfisher87
Copy link
Member Author

TODO: Unit test!

@mfisher87 mfisher87 force-pushed the python-api-remove-layer branch from 9ea7c6d to 758fa20 Compare February 19, 2025 21:41
@brichet
Copy link
Collaborator

brichet commented Feb 20, 2025

Thanks @mfisher87 for working on this, I thought it was already available, we've probably been talking about it since the very beginning of the Python API 😃

TODO: Unit test!

👍

@brichet
Copy link
Collaborator

brichet commented Feb 20, 2025

Some API functions use the layer id, but I don't know if there is a way to easily display/copy that id from the UI.
Maybe we could add this in the property panel in a follow up PR.

if layer is None:
raise KeyError(f"No layer found with ID: {layer_id}")

del self._layers[layer_id]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also remove the corresponding source if it is not used by another layer ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems sensible! I'll do a bit of test-driven development today (or more realistically, tomorrow. No meetings on Fridays!) :)

@mfisher87
Copy link
Member Author

Thanks @mfisher87 for working on this, I thought it was already available, we've probably been talking about it since the very beginning of the Python API 😃

Happy to! Main credit to @YaoTingYao :)

@mfisher87 mfisher87 marked this pull request as draft February 20, 2025 22:05
@mfisher87
Copy link
Member Author

Unit tests added. TODO: also remove orphan source!

@mfisher87
Copy link
Member Author

@brichet I would really like one more test: Add two layers from the same source, then remove one layer and check that there is still 1 layer and 1 source afterwards. I struggled with how to add multiple layers from the same source in the Python API. Am I perhaps missing an existing mechanism for this?

@mfisher87 mfisher87 force-pushed the python-api-remove-layer branch from 5071244 to ce88ce4 Compare February 21, 2025 01:43
@mfisher87 mfisher87 marked this pull request as ready for review February 21, 2025 01:43
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!!

@martinRenou martinRenou merged commit 27a6f56 into geojupyter:main Feb 21, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for Removing Layers in GISDocument from python api
4 participants