Closed cleder closed 1 week ago
This PR adds support for ScreenOverlay in the fastkml library. The implementation includes new classes for handling screen coordinates and overlay positioning, along with comprehensive test coverage. The changes follow the existing patterns in the codebase for KML element handling.
classDiagram
class _XMLObject {
}
class _XY {
+Optional~float~ x
+Optional~float~ y
+Optional~Units~ x_units
+Optional~Units~ y_units
+__init__(Optional~str~ ns, Optional~Dict~ name_spaces, Optional~float~ x, Optional~float~ y, Optional~Units~ x_units, Optional~Units~ y_units)
+__repr__() str
+__bool__() bool
}
_XY --|> _XMLObject
class OverlayXY {
+get_tag_name() str
}
OverlayXY --|> _XY
class ScreenXY {
+get_tag_name() str
}
ScreenXY --|> _XY
class RotationXY {
+get_tag_name() str
}
RotationXY --|> _XY
class Size {
+get_tag_name() str
}
Size --|> _XY
class _Overlay {
}
class ScreenOverlay {
+Optional~OverlayXY~ overlay_xy
+Optional~ScreenXY~ screen_xy
+Optional~RotationXY~ rotation_xy
+Optional~Size~ size
+Optional~float~ rotation
+__init__(Optional~str~ ns, Optional~Dict~ name_spaces, Optional~str~ id, Optional~str~ target_id, Optional~str~ name, Optional~bool~ visibility, Optional~bool~ isopen, Optional~atom.Link~ atom_link, Optional~atom.Author~ atom_author, Optional~str~ address, Optional~str~ phone_number, Optional~Snippet~ snippet, Optional~str~ description, Optional~Union~ view, Optional~Union~ times, Optional~StyleUrl~ style_url, Optional~Iterable~ styles, Optional~Region~ region, Optional~ExtendedData~ extended_data, Optional~str~ color, Optional~int~ draw_order, Optional~Icon~ icon, Optional~OverlayXY~ overlay_xy, Optional~ScreenXY~ screen_xy, Optional~RotationXY~ rotation_xy, Optional~Size~ size, Optional~float~ rotation)
+__repr__() str
}
ScreenOverlay --|> _Overlay
_Overlay <|-- ScreenOverlay
note for ScreenOverlay "Handles screen overlay elements in KML"
Change | Details | Files |
---|---|---|
Added new base class for handling screen coordinates |
|
fastkml/overlays.py |
Implemented ScreenOverlay and related positioning classes |
|
fastkml/overlays.py |
Added test coverage for new overlay functionality |
|
tests/overlays_test.py tests/hypothesis/overlay_test.py tests/hypothesis/strategies.py |
Updated container support for ScreenOverlay |
|
fastkml/containers.py |
The pull request introduces significant updates to the fastkml
library, including the addition of support for ScreenOverlay
. Version 1.0.0dev0
is noted in the changelog, followed by version 1.0
, which drops Python 2 support, mandates pygeoif
version 1.0 or higher, and removes native support for shapely
. The codebase has been refactored with added type annotations, replaced the dateutil
library with arrow
, and introduced an informative __repr__
method. New classes related to overlays are defined, and tests for these functionalities are enhanced.
File Path | Change Summary |
---|---|
docs/HISTORY.rst | Updated changelog for version 1.0.0dev0 (added support for ScreenOverlay ) and version 1.0 (dropped Python 2, etc.). |
fastkml/containers.py | Added import for ScreenOverlay , updated __all__ , modified _Container registration to include ScreenOverlay . |
fastkml/overlays.py | Introduced classes _XY , OverlayXY , ScreenXY , RotationXY , Size , and ScreenOverlay , with updated constructors. |
tests/hypothesis/overlay_test.py | Added parameterized tests for overlay classes and a test for ScreenOverlay . |
tests/hypothesis/strategies.py | Introduced new partial strategy xy for generating coordinate pairs with units. |
tests/overlays_test.py | Created TestScreenOverlay class with a method for testing ScreenOverlay creation from KML string; renamed existing classes. |
fastkml/containers.py
regarding the ScreenOverlay
class are directly related to the main PR's addition of support for ScreenOverlay
.fastkml/overlays.py
file, which is relevant as the main PR also involves changes to overlays, including the addition of ScreenOverlay
.enhancement
, Tests
, documentation
In the land of KML, a new feature's born,
WithScreenOverlay
, our hearts are worn.
Python 2's gone, we embrace the new,
Type annotations shine, like morning dew.
With tests all ready, our code's set to play,
Hopping into version 1.0, hip-hip-hooray! ๐โจ
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Here are some key observations to aid the review process:
โฑ๏ธ Estimated effort to review: 4 ๐ต๐ต๐ต๐ตโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Recommended focus areas for review Input Validation The _XY class and its derivatives (OverlayXY, ScreenXY, etc.) accept any float values for x and y coordinates without validation. Should validate that values are within reasonable bounds. Default Values The ScreenOverlay class has many optional parameters but lacks default values for key positioning attributes like overlay_xy and screen_xy. Consider adding sensible defaults. Error Handling The __bool__ method in _XY class only checks if x and y are not None, but doesn't validate units. Should handle invalid unit combinations. |
Preparing review...
Explore these optional code suggestions:
Category | Suggestion | Score |
Possible issue |
Add validation for coordinate values when using fractional units___ **Add validation in _XY class to ensure x and y coordinates are valid when units areset to 'fraction' (must be between 0 and 1).** [fastkml/overlays.py [1290-1299]](https://github.com/cleder/fastkml/pull/393/files#diff-53d4a4402d4b36ba24c38b2494214f2940433d433946065052596dbd37cc3565R1290-R1299) ```diff def __init__( self, ns: Optional[str] = None, name_spaces: Optional[Dict[str, str]] = None, x: Optional[float] = None, y: Optional[float] = None, x_units: Optional[Units] = None, y_units: Optional[Units] = None, **kwargs: Any, ) -> None: + if x is not None and x_units == Units.fraction and not 0 <= x <= 1: + raise ValueError("x must be between 0 and 1 when units is fraction") + if y is not None and y_units == Units.fraction and not 0 <= y <= 1: + raise ValueError("y must be between 0 and 1 when units is fraction") ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: This validation is critical for preventing invalid coordinate values when using fractional units, which must be between 0 and 1. Missing this validation could lead to incorrect overlay positioning and rendering errors. | 9 |
Add validation to ensure rotation values stay within valid bounds___ **Add validation for rotation value to ensure it stays within valid range of -180 to180 degrees. Currently the rotation parameter can accept any float value.** [fastkml/overlays.py [1451-1458]](https://github.com/cleder/fastkml/pull/393/files#diff-53d4a4402d4b36ba24c38b2494214f2940433d433946065052596dbd37cc3565R1451-R1458) ```diff def __init__( self, ns: Optional[str] = None, name_spaces: Optional[Dict[str, str]] = None, ... rotation: Optional[float] = None, **kwargs: Any, ) -> None: + if rotation is not None and not -180 <= rotation <= 180: + raise ValueError("rotation must be between -180 and 180 degrees") ``` Suggestion importance[1-10]: 8Why: The suggestion adds important validation to prevent invalid rotation values that could cause rendering issues or unexpected behavior. The -180 to 180 degree constraint is a standard range for rotation angles. | 8 |
๐ก Need additional feedback ? start a PR chat
Failed to generate code suggestions for PR
**Action:** SonarCloud |
**Failed stage:** [SonarCloud Scan](https://github.com/cleder/fastkml/actions/runs/11996447472/job/33440897512) [โ] |
**Failure summary:**
The action failed because there was an error while trying to retrieve the pull request with key '393'. This error occurred during the auto-configuration process for the pull request. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 260: 13:13:12.052 INFO Check ALM binding of project 'cleder_fastkml' 261: 13:13:12.525 INFO Detected project binding: BOUND 262: 13:13:12.525 INFO Check ALM binding of project 'cleder_fastkml' (done) | time=472ms 263: 13:13:12.526 INFO Load project pull requests 264: 13:13:13.000 INFO Load project pull requests (done) | time=473ms 265: 13:13:13.001 INFO Load branch configuration 266: 13:13:13.002 INFO Github event: pull_request 267: 13:13:13.009 INFO Auto-configuring pull request 393 268: 13:13:13.526 ERROR Something went wrong while trying to get the pullrequest with key '393' 269: 13:13:13.850 INFO EXECUTION FAILURE ``` |
Preparing review...
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
a6b5087
) to head (d836ecf
). Report is 6 commits behind head on develop.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
๐จ Try these New Features:
Preparing review...
User description
PR Type
enhancement, tests
Description
ScreenOverlay
class and related functionality in thefastkml
library._XY
class and its derivatives (OverlayXY
,ScreenXY
,RotationXY
,Size
) for handling screen positioning.ScreenOverlay
and related classes using hypothesis.ScreenOverlay
support.Changes walkthrough ๐
containers.py
Add ScreenOverlay support to containers module
fastkml/containers.py
ScreenOverlay
.__all__
to includeScreenOverlay
.ScreenOverlay
in the registry.overlays.py
Implement ScreenOverlay and related classes for KML overlays
fastkml/overlays.py
_XY
class for screen positioning.OverlayXY
,ScreenXY
,RotationXY
, andSize
classes.ScreenOverlay
class with detailed initialization.overlay_test.py
Add hypothesis tests for ScreenOverlay and related classes
tests/hypothesis/overlay_test.py
OverlayXY
,RotationXY
,ScreenXY
, andSize
.ScreenOverlay
.strategies.py
Add hypothesis strategy for OverlayXY related classes
tests/hypothesis/strategies.py - Added `xy` strategy for generating `OverlayXY` related test data.
overlays_test.py
Add tests for ScreenOverlay functionality
tests/overlays_test.py
ScreenOverlay
creation from string.ScreenOverlay
.HISTORY.rst
Update changelog for ScreenOverlay support
docs/HISTORY.rst - Updated changelog to include ScreenOverlay support.
Summary by CodeRabbit
New Features
ScreenOverlay
, enhancing KML functionality.Bug Fixes
ScreenOverlay
tests.Documentation
Tests
ScreenOverlay
andGroundOverlay
tests.