1.0.alpha 13 #358
1.0.alpha 13 #358
Conversation
updates: - [github.com/astral-sh/ruff-pre-commit: v0.4.9 → v0.4.10](astral-sh/ruff-pre-commit@v0.4.9...v0.4.10)
[pre-commit.ci] pre-commit autoupdate
updates: - [github.com/astral-sh/ruff-pre-commit: v0.4.10 → v0.5.0](astral-sh/ruff-pre-commit@v0.4.10...v0.5.0) - [github.com/pre-commit/mirrors-mypy: v1.10.0 → v1.10.1](pre-commit/mirrors-mypy@v1.10.0...v1.10.1) - [github.com/adamchainz/blacken-docs: 1.16.0 → 1.18.0](adamchainz/blacken-docs@1.16.0...1.18.0)
for more information, see https://pre-commit.ci
[pre-commit.ci] pre-commit autoupdate
updates: - [github.com/astral-sh/ruff-pre-commit: v0.5.0 → v0.5.1](astral-sh/ruff-pre-commit@v0.5.0...v0.5.1) - [github.com/rstcheck/rstcheck: v6.2.0 → v6.2.4](rstcheck/rstcheck@v6.2.0...v6.2.4)
[pre-commit.ci] pre-commit autoupdate
updates: - [github.com/astral-sh/ruff-pre-commit: v0.5.1 → v0.5.2](astral-sh/ruff-pre-commit@v0.5.1...v0.5.2)
[pre-commit.ci] pre-commit autoupdate
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…st-usage-guide-is-broken-for-version-1x add docstrings and fixes for ruff
updates: - [github.com/astral-sh/ruff-pre-commit: v0.5.2 → v0.5.4](astral-sh/ruff-pre-commit@v0.5.2...v0.5.4) - [github.com/pre-commit/mirrors-mypy: v1.10.1 → v1.11.0](pre-commit/mirrors-mypy@v1.10.1...v1.11.0)
[pre-commit.ci] pre-commit autoupdate
Refactor KML class and related styles and geometries This commit refactors the KML class in the fastkml module. The `_features` attribute has been renamed to `features` to follow naming conventions. In the styles module, the `LineStyle` and `PolyStyle` classes have been modified. The `__bool__` method in `LineStyle` now checks for the presence of `width`, `color`, and `color_mode` attributes. Similarly, the `__bool__` method in `PolyStyle` now checks for the presence of `fill`, `outline`, `color`, and `color_mode` attributes. The coordinates_test module in the tests directory has been updated with a new test case. The `test_coordinates_from_string_with_whitespace` method tests the `from_string` method with whitespace. Lastly, the geometry module has been modified. The `OuterBoundaryIs` and `InnerBoundaryIs` classes now have `__repr__` methods to provide a string representation of the objects.
updates: - [github.com/astral-sh/ruff-pre-commit: v0.6.5 → v0.6.7](astral-sh/ruff-pre-commit@v0.6.5...v0.6.7)
[pre-commit.ci] pre-commit autoupdate
updates: - [github.com/abravalheri/validate-pyproject: v0.19 → v0.20.2](abravalheri/validate-pyproject@v0.19...v0.20.2) - [github.com/astral-sh/ruff-pre-commit: v0.6.7 → v0.6.8](astral-sh/ruff-pre-commit@v0.6.7...v0.6.8)
[pre-commit.ci] pre-commit autoupdate
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…operly Fix inner boundaries for polygon #355
|
Review changes with SemanticDiff. Analyzed 31 of 38 files. Overall, the semantic diff is 4% smaller than the GitHub diff. File Information
|
Reviewer's Guide by SourceryThis pull request implements several improvements and updates to the fastkml library. The changes focus on code quality, performance enhancements, and adherence to best practices. Key modifications include updating namespace handling, improving type hinting, enhancing documentation, and refactoring various components for better maintainability and functionality. Updated class diagram for fastkml.geometry moduleclassDiagram
class Coordinates {
-_default_nsid: config.KML
-coords: LineType
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], coords: Optional[LineType], **kwargs: Any) -> None
+__bool__() bool
}
class OuterBoundaryIs {
-_default_nsid: config.KML
-kml_geometry: Optional[LinearRing]
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], geometry: Optional[geo.LinearRing], kml_geometry: Optional[LinearRing], **kwargs: Any) -> None
+__bool__() bool
+geometry() Optional[geo.LinearRing]
}
class InnerBoundaryIs {
-_default_nsid: config.KML
-kml_geometry: Optional[LinearRing]
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], geometry: Optional[geo.LinearRing], kml_geometry: Optional[LinearRing], **kwargs: Any) -> None
+__bool__() bool
+geometry() Optional[geo.LinearRing]
}
class Polygon {
-outer_boundary: Optional[OuterBoundaryIs]
-inner_boundaries: List[InnerBoundaryIs]
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], id: Optional[str], target_id: Optional[str], extrude: Optional[bool], tessellate: Optional[bool], altitude_mode: Optional[AltitudeMode], outer_boundary: Optional[OuterBoundaryIs], inner_boundaries: Optional[Iterable[InnerBoundaryIs]], geometry: Optional[geo.Polygon], **kwargs: Any) -> None
+__bool__() bool
+geometry() Optional[geo.Polygon]
}
Updated class diagram for fastkml.styles moduleclassDiagram
class Style {
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], id: Optional[str], target_id: Optional[str], url: Optional[str], **kwargs: Any) -> None
+__bool__() bool
}
class _ColorStyle {
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], id: Optional[str], target_id: Optional[str], color: Optional[str], color_mode: Optional[ColorMode], **kwargs: Any) -> None
}
class HotSpot {
-_default_nsid: config.KML
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], x: Optional[float], y: Optional[float], xunits: Optional[Units], yunits: Optional[Units], **kwargs: Any) -> None
+__bool__() bool
}
Updated class diagram for fastkml.features moduleclassDiagram
class _Feature {
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], id: Optional[str], target_id: Optional[str], name: Optional[str], visibility: Optional[bool], isopen: Optional[bool], atom_link: Optional[atom.Link], atom_author: Optional[atom.Author], address: Optional[str], phone_number: Optional[str], snippet: Optional[Snippet], description: Optional[str], view: Optional[Union[Camera, LookAt]], times: Optional[Union[TimeSpan, TimeStamp]], style_url: Optional[StyleUrl], styles: Optional[Iterable[Union[Style, StyleMap]]], region: Optional[Region], extended_data: Optional[ExtendedData], **kwargs: Any) -> None
}
class Placemark {
+__init__(ns: Optional[str], name_spaces: Optional[Dict[str, str]], id: Optional[str], target_id: Optional[str], name: Optional[str], visibility: Optional[bool], isopen: Optional[bool], atom_link: Optional[atom.Link], atom_author: Optional[atom.Author], address: Optional[str], phone_number: Optional[str], snippet: Optional[Snippet], description: Optional[str], view: Optional[Union[Camera, LookAt]], times: Optional[Union[TimeSpan, TimeStamp]], style_url: Optional[StyleUrl], styles: Optional[Iterable[Union[Style, StyleMap]]], region: Optional[Region], extended_data: Optional[ExtendedData], kml_geometry: KmlGeometry, geometry: Optional[Union[GeoType, GeoCollectionType]], **kwargs: Any) -> None
+geometry() Optional[AnyGeometryType]
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several updates across multiple files, including modifications to the testing workflow to support additional Python versions and adjustments to dependency installations. The Changes
Possibly related PRs
Suggested labels
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Hello @cleder! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Summary
|
|
Preparing review... |
1 similar comment
|
Preparing review... |
|
Failed to generate code suggestions for PR |
PR Code Suggestions ✨Explore these optional code suggestions:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 3ecc272 in 2 minutes and 7 seconds
More details
- Looked at
8447lines of code in38files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/styles_test.py:416
- Draft comment:
The order of styles in the assertion does not match the serialized order. Ensure the order of assertions matches the order in the serialized XML. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_qrRBxMdS7MUFfZNQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cleder - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert not polygon.geometry | ||
| assert polygon.outer_boundary_is is not None | ||
| assert polygon.outer_boundary is not None | ||
| assert isinstance(polygon.outer_boundary, OuterBoundaryIs) | ||
| assert len(polygon.inner_boundaries) == 0 |
There was a problem hiding this comment.
suggestion (testing): Update assertions to match new Polygon structure
The test has been updated to check the new structure of the Polygon class. It now asserts that outer_boundary is not None and is an instance of OuterBoundaryIs, and that inner_boundaries is an empty list. This change reflects the new implementation of the Polygon class.
| assert not polygon.geometry | |
| assert polygon.outer_boundary_is is not None | |
| assert polygon.outer_boundary is not None | |
| assert isinstance(polygon.outer_boundary, OuterBoundaryIs) | |
| assert len(polygon.inner_boundaries) == 0 | |
| assert polygon.outer_boundary is not None | |
| assert isinstance(polygon.outer_boundary, OuterBoundaryIs) | |
| assert polygon.inner_boundaries == [] |
| assert polygon.outer_boundary_is is not None | ||
| assert polygon.outer_boundary is not None | ||
| assert isinstance(polygon.outer_boundary, OuterBoundaryIs) | ||
| assert len(polygon.inner_boundaries) == 0 | ||
| assert "tessellate>1</" in polygon.to_string() |
There was a problem hiding this comment.
suggestion (testing): Add test for new Polygon attributes
Consider adding assertions to check other attributes of the Polygon class, such as extrude, tessellate, and altitude_mode. This would provide more comprehensive coverage of the Polygon class's structure.
import pygeoif.geometry as geo
from fastkml.geometry import OuterBoundaryIs, Polygon
from tests.base import Lxml, StdLibrary
polygon = Polygon.class_from_string(doc)
assert not polygon.geometry
assert polygon.outer_boundary is not None
assert isinstance(polygon.outer_boundary, OuterBoundaryIs)
assert len(polygon.inner_boundaries) == 0
assert "tessellate>1</" in polygon.to_string()
assert polygon.extrude == 0
assert polygon.altitude_mode == "clampToGround"
| def test_repr(self) -> None: | ||
| """Test the __repr__ method.""" | ||
| new_doc = eval(repr(self.clean_doc), {}, eval_locals) # noqa: S307 | ||
|
|
||
| assert new_doc == self.clean_doc | ||
| assert repr(new_doc) == repr(self.clean_doc) |
There was a problem hiding this comment.
suggestion (testing): Add more specific assertions for repr test
The test_repr method could be more thorough. Consider adding assertions to check specific attributes or structures within the repr output, not just equality of the entire repr.
| def test_repr(self) -> None: | |
| """Test the __repr__ method.""" | |
| new_doc = eval(repr(self.clean_doc), {}, eval_locals) # noqa: S307 | |
| assert new_doc == self.clean_doc | |
| assert repr(new_doc) == repr(self.clean_doc) | |
| def test_repr(self) -> None: | |
| """Test the __repr__ method.""" | |
| new_doc = eval(repr(self.clean_doc), {}, eval_locals) # noqa: S307 | |
| assert new_doc == self.clean_doc | |
| assert repr(new_doc) == repr(self.clean_doc) | |
| assert isinstance(new_doc, type(self.clean_doc)) | |
| assert new_doc.text == self.clean_doc.text | |
| assert new_doc.metadata == self.clean_doc.metadata |
| def test_eq_str_round_trip(self) -> None: | ||
| """Test the equality of the original and the round-tripped document.""" | ||
| new_doc = fastkml.KML.class_from_string(self.clean_doc.to_string(precision=15)) | ||
|
|
||
| assert str(self.clean_doc) == str(new_doc) | ||
| assert repr(new_doc) == repr(self.clean_doc) | ||
| # srict equality is not a given new_doc == self.clean_doc |
There was a problem hiding this comment.
issue (testing): Incomplete equality test in round-trip conversion
The test_eq_str_round_trip method doesn't actually test for equality between new_doc and self.clean_doc. Consider adding an assertion to check for equality, or explain why strict equality is not expected.
| class TestReprLxml(Lxml, TestRepr): | ||
| """Test the __repr__ and __str__ methods of the KML document with lxml.""" |
There was a problem hiding this comment.
suggestion (testing): Consider adding specific lxml tests
The TestReprLxml class doesn't contain any lxml-specific tests. Consider adding tests that specifically check lxml functionality or behavior differences compared to the standard library implementation.
| class TestReprLxml(Lxml, TestRepr): | |
| """Test the __repr__ and __str__ methods of the KML document with lxml.""" | |
| class TestReprLxml(Lxml, TestRepr): | |
| """Test the __repr__ and __str__ methods of the KML document with lxml.""" | |
| def test_lxml_specific_repr(self): | |
| doc = self.kml.Document() | |
| self.assertIn('lxml.etree', repr(doc)) | |
| def test_lxml_specific_str(self): | |
| doc = self.kml.Document() | |
| self.assertIn('<?xml', str(doc)) |
| import fastkml | ||
| from fastkml.enums import AltitudeMode | ||
| from fastkml.enums import PairKey | ||
| from tests.base import Lxml |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
| from fastkml.enums import AltitudeMode | ||
| from fastkml.enums import PairKey | ||
| from tests.base import Lxml | ||
| from tests.base import StdLibrary |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
Important
Enhance test coverage for
fastkmllibrary by adding and updating tests for geometry, styles, and KML parsing.OuterBoundaryIsandInnerBoundaryIsinboundaries_test.py.Coordinateshandling whitespace incoordinates_test.py.Point,LineString,Polygoningeometry_test.py.polygon_test.py.kml_test.py.__repr__and__eq__methods inrepr_eq_test.py.styles_test.py.This description was created by
for 3ecc272. It will automatically update as commits are pushed.
Summary by Sourcery
Enhance the codebase with improved documentation, refactorings for better namespace handling, and expanded test coverage. Update dependencies and pre-commit hooks to their latest versions.
Enhancements:
KMLclass to include aparsemethod for parsing KML files from different input types, enhancing flexibility.__repr__and__eq__methods for several classes to ensure accurate string representation and equality checks._default_nsidand updating namespace management logic.testsuite with additional tests for parsing KML files and verifying the__repr__and__eq__methods.Tests:
__repr__and__eq__methods to ensure accurate object representation and equality checks.Chores:
_typos.tomlconfiguration file to manage custom dictionary entries for typo checks.Summary by CodeRabbit
New Features
Documentation
README.rstfor terminology consistency and clarified attribute assignments.docs/contributing.rstto improve clarity on setting up the development environment.docs/HISTORY.rstto reflect significant project changes and version updates.Bug Fixes
fastkmlpackage.Refactor