Release 1.0.0b2 #381
Release 1.0.0b2 #381
Conversation
… end in time tests
374 add hypothesis tests for times and views
374 add hypothesis tests for data and styles
Reviewer's Guide by SourceryThis pull request implements several significant changes to improve code quality, add property-based testing, and prepare for release 1.0.0b2. The changes focus on refactoring class hierarchies, improving type handling, adding extensive property-based tests, and enhancing CI/CD configuration. Updated class diagram for SimpleField and Schema classesclassDiagram
class _BaseObject
class _XMLObject
class SimpleField {
+Optional~str~ name
+Optional~DataType~ type_
+Optional~str~ display_name
+__init__(Optional~str~ ns, Optional~Dict~str, str~ name_spaces, Optional~str~ name, Optional~DataType~ type_, Optional~str~ display_name, **kwargs)
+__repr__() str
+__bool__() bool
}
class Schema {
+Optional~str~ name
+List~SimpleField~ fields
+__init__(Optional~str~ ns, Optional~Dict~str, str~ name_spaces, Optional~str~ id, Optional~str~ name, Optional~Iterable~SimpleField~ fields, **kwargs)
+__repr__() str
+append(SimpleField field)
}
_BaseObject <|-- _XMLObject
_XMLObject <|-- SimpleField
_XMLObject <|-- Schema
Updated class diagram for StyleUrl and IconStyle classesclassDiagram
class _BaseObject
class _XMLObject
class StyleUrl {
+Optional~str~ url
+__init__(Optional~str~ ns, Optional~Dict~str, str~ name_spaces, Optional~str~ url, **kwargs)
+__repr__() str
}
class IconStyle {
+Optional~float~ scale
+Optional~float~ heading
+Optional~Icon~ icon
+Optional~HotSpot~ hot_spot
+__init__(Optional~str~ ns, Optional~Dict~str, str~ name_spaces, Optional~str~ id, Optional~str~ target_id, Optional~str~ color, Optional~ColorMode~ color_mode, Optional~float~ scale, Optional~float~ heading, Optional~Icon~ icon, Optional~HotSpot~ hot_spot, **kwargs)
+__repr__() str
+__bool__() bool
+icon_href() Optional~str~
}
_BaseObject <|-- _XMLObject
_XMLObject <|-- StyleUrl
_XMLObject <|-- IconStyle
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe pull request introduces significant updates across multiple files, primarily enhancing the testing framework and documentation for the Fastkml library. Key changes include modifications to GitHub Actions workflows for improved test coverage reporting and the addition of a new SonarCloud integration workflow. The pre-commit configuration has been updated to specify Python 3.12 and adjust hooks. Documentation has been refined for clarity, and several classes have undergone structural changes, including optional attributes and improved error handling. New tests have been added, particularly focusing on property-based testing using Hypothesis. 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
|
PR Summary
|
CI Failure Feedback 🧐
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
|
Failed to generate code suggestions for PR |
![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 d0c318f in 51 seconds
More details
- Looked at
3839lines of code in38files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/times_test.py:295
- Draft comment:
Thetest_parse_datetime_no_tztest case assumes UTC when no timezone is provided. This might not be the intended behavior, as the absence of a timezone should not default to UTC without explicit documentation or requirements stating so. Consider clarifying the expected behavior or adjusting the test case. - Reason this comment was not posted:
Confidence changes required:50%
The test files are well-structured and follow good practices. However, there is a minor issue in thetests/times_test.pyfile where thetest_parse_datetime_no_tztest case assumes UTC when no timezone is provided. This might not be the intended behavior, as the absence of a timezone should not default to UTC without explicit documentation or requirements stating so.
Workflow ID: wflow_jJzFICRP6lXvqkO7
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: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
| self.name = name | ||
| self.type = type | ||
| self.display_name = display_name | ||
| self.name = name.strip() or None if name else None |
There was a problem hiding this comment.
issue (complexity): Consider extracting the repeated string validation pattern into a utility function.
The repeated string validation pattern value.strip() or None if value else None should be extracted to a utility function to improve maintainability and reduce duplication. Suggested change:
def clean_string(value: Optional[str]) -> Optional[str]:
"""Clean and validate a string value, returning None if empty."""
return value.strip() or None if value else None
class SimpleField(_XMLObject):
def __init__(self, ..., name: Optional[str] = None, ...):
super().__init__(...)
self.name = clean_string(name)
self.display_name = clean_string(display_name)This change:
- Reduces code duplication
- Makes string handling consistent across all classes
- Improves maintainability by centralizing the logic
- Makes the validation behavior more explicit through the function name
| import fastkml | ||
| import fastkml.data | ||
| import fastkml.enums | ||
| 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.
| import fastkml.data | ||
| import fastkml.enums | ||
| from tests.base import Lxml | ||
| from tests.hypothesis.common import assert_repr_roundtrip |
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.
| import fastkml.enums | ||
| from tests.base import Lxml | ||
| from tests.hypothesis.common import assert_repr_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip |
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 tests.base import Lxml | ||
| from tests.hypothesis.common import assert_repr_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip_terse |
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.
| import fastkml | ||
| import fastkml.enums | ||
| import fastkml.views | ||
| 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.
| import fastkml.enums | ||
| import fastkml.views | ||
| from tests.base import Lxml | ||
| from tests.hypothesis.common import assert_repr_roundtrip |
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.
| import fastkml.views | ||
| from tests.base import Lxml | ||
| from tests.hypothesis.common import assert_repr_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip |
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 tests.base import Lxml | ||
| from tests.hypothesis.common import assert_repr_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip_terse |
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 tests.hypothesis.common import assert_repr_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip | ||
| from tests.hypothesis.common import assert_str_roundtrip_terse | ||
| from tests.hypothesis.common import assert_str_roundtrip_verbose |
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.
|
Preparing review... |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Preparing review... |
PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (43)
.github/workflows/sonarcloud.yml (1)
1-7: LGTM! Consider adding YAML document start marker.The workflow trigger configuration is well-structured, appropriately targeting both the develop branch and pull request events.
Add the YAML document start marker for better standardization:
+--- name: SonarBuild🧰 Tools
🪛 GitHub Check: static-tests (3.12)
[warning] 1-1:
1:1 [document-start] missing document start "---"
[warning] 2-2:
2:1 [truthy] truthy value should be one of [false, true]fastkml/utils.py (1)
Line range hint
67-89: Excellent implementation with room for documentation improvementsThe implementation is clean, efficient, and follows Python best practices. Consider enhancing the docstring with:
- Usage examples
- A note about its relationship with
find_all- Performance characteristics (stops after finding first match)
Here's a suggested docstring enhancement:
""" Find the first instance of a given type in a given object. + + This is an optimized version of find_all() that stops after finding the first match. + It uses the same search logic but is more efficient when only one result is needed. + Args: ---- obj: The object to search. of_type: The type(s) to search for or None for any type. **kwargs: Attributes of the object to match. Returns: ------- The first instance of the given type in the given object or None if not found. + Examples: + -------- + >>> from fastkml import KML + >>> k = KML() + >>> # Find first Placemark + >>> placemark = find(k, of_type=Placemark) + >>> # Find first element with specific name + >>> named_element = find(k, name="Example") """tests/hypothesis/times_test.py (1)
39-57: Consider enhancing test coverage with additional assertions.While the current implementation thoroughly tests string representations, consider adding:
- Assertions to verify that the stored values match the input parameters
- Test cases for invalid timestamp scenarios to ensure proper error handling
Example additions:
def test_fuzz_time_stamp( self, id: typing.Optional[str], target_id: typing.Optional[str], timestamp: typing.Optional[fastkml.times.KmlDateTime], ) -> None: time_stamp = fastkml.TimeStamp(id=id, target_id=target_id, timestamp=timestamp) # Verify stored values assert time_stamp.id == id assert time_stamp.target_id == target_id assert time_stamp.timestamp == timestamp # Existing assertions assert_repr_roundtrip(time_stamp) assert_str_roundtrip(time_stamp) assert_str_roundtrip_terse(time_stamp) assert_str_roundtrip_verbose(time_stamp)tests/hypothesis/gx_test.py (1)
Line range hint
52-116: Consider adding edge case tests.The property-based tests are well-structured, but consider adding explicit edge cases:
- Empty track items list
- Single track item
- Maximum/minimum float values for Angle parameters
- Nested MultiTracks with varying depths
Here's a suggested test addition:
def test_edge_cases(self): # Empty track track = fastkml.gx.Track(track_items=[]) assert_str_roundtrip(track) # Single item track track = fastkml.gx.Track(track_items=[ TrackItem( angle=Angle(heading=0.0, roll=0.0, tilt=0.0), coord=Point(0, 0), when=kml_datetimes().example() ) ]) assert_str_roundtrip(track).github/workflows/run-all-tests.yml (2)
Line range hint
38-47: Consider documenting the coverage threshold decision.The 95% coverage threshold is quite high, which ensures good test coverage but might be challenging to maintain. Consider documenting the rationale for this threshold in the repository's documentation.
Good practice on uploading coverage only for Python 3.11 to avoid duplicate reports.
73-73: Consider conditional verbosity for doctest.While removing
-vmakes the logs cleaner, it might make debugging failures harder. Consider making verbosity conditional:- python -m doctest docs/*.rst + python -m doctest docs/*.rst ${{ github.event_name == 'pull_request' && '-v' || '' }}This would enable verbose output only during PR checks when detailed feedback is most valuable.
tests/atom_test.py (1)
148-150: LGTM! Consider adding edge case tests.The assertion change correctly validates that missing name elements deserialize to
None. This aligns with the updated behavior in the_Personclass.Consider adding tests for edge cases like:
- Empty name element (
<atom:name></atom:name>)- Name element with whitespace (
<atom:name> </atom:name>)fastkml/links.py (3)
81-81: Add type validation for href parameter.While stripping whitespace is a good improvement, consider adding type validation to prevent potential AttributeError if a non-string object is passed.
- self.href = href.strip() if href else "" + self.href = href.strip() if isinstance(href, str) else ""
Line range hint
196-207: Consider implementing the TODO for icon palette support.The TODO comment about implementing
gx:x,gx:y,gx:w, andgx:helements for icon palette support has been present for a while. This functionality could be important for users working with icon palettes.Would you like me to help implement this feature or create a GitHub issue to track this enhancement?
Line range hint
77-89: Consider adding URL validation.The Link class could benefit from basic URL format validation in the constructor to catch invalid URLs early.
Here's a suggested implementation:
from urllib.parse import urlparse def __init__(self, ...) -> None: super().__init__(...) # Validate and clean href if href: href = href.strip() try: result = urlparse(href) if not all([result.scheme, result.netloc]) and not result.path: raise ValueError(f"Invalid URL format: {href}") except Exception as e: raise ValueError(f"Invalid URL: {href}") from e self.href = href if href else ""tests/hypothesis/data_test.py (4)
16-16: Enhance module docstring with more context.The current docstring could be more descriptive about the purpose and benefits of property-based testing in this context.
Consider expanding it to something like:
-"""Property-based tests for the views module.""" +"""Property-based tests for the fastkml data structures. + +This module uses Hypothesis to generate a wide range of test cases, +ensuring robustness of the fastkml data classes through property-based testing. +It validates serialization, deserialization, and representation of various +KML data structures."""
36-42: Add type hints and consider strategy reusability.The
simple_fieldsstrategy is well-defined but could benefit from type hints and better reusability.Consider this improvement:
+SimpleFieldStrategy = st.SearchStrategy[fastkml.data.SimpleField] + simple_fields = partial( st.builds, fastkml.data.SimpleField, name=xml_text().filter(lambda x: x.strip() != ""), type_=st.one_of(st.sampled_from(fastkml.enums.DataType)), display_name=xml_text().filter(lambda x: x.strip() != ""), -) +) -> SimpleFieldStrategy
73-89: Add field order preservation test.The schema test should verify that the order of fields is preserved after serialization/deserialization.
Add this assertion after line 88:
if fields: assert [f.name for f in schema.fields] == [f.name for f in fields]
203-209: Extract sorting logic and add empty elements test.The sorting logic in
test_fuzz_extended_datacould be clearer and the empty case should be explicitly tested.Consider these improvements:
+def sort_extended_data_elements(elements): + """Sort ExtendedData elements by class name.""" + return sorted(elements, key=lambda t: t.__class__.__name__) if elements else None extended_data = fastkml.ExtendedData( - elements=( - sorted(elements, key=lambda t: t.__class__.__name__) - if elements - else None - ), + elements=sort_extended_data_elements(elements), ) +# Verify empty elements case +assert extended_data.elements == [] if elements is None else extended_data.elements is not Nonedocs/working_with_kml.rst (1)
214-215: Consider adding BalloonStyle documentation.While the output shows BalloonStyle elements, it would be helpful to add a brief explanation of what BalloonStyle does and why it's included in the transformed styles.
Also applies to: 228-230, 241-241
docs/conf.py (1)
179-185: LGTM! Consider extracting GitHub configuration to environment variables.The GitHub integration configuration is correctly implemented and will enable "Edit on GitHub" links in the documentation. This is particularly useful for community contributions.
For improved maintainability, consider extracting these values to environment variables or a separate configuration file, especially if you plan to reuse them in other parts of the project (e.g., CI/CD workflows).
Example refactor:
html_context = { "display_github": True, # Integrate GitHub - "github_user": "cleder", # Username - "github_repo": "fastkml", # Repo name - "github_version": "main", # Version + "github_user": os.getenv("GITHUB_USER", "cleder"), # Username + "github_repo": os.getenv("GITHUB_REPO", "fastkml"), # Repo name + "github_version": os.getenv("GITHUB_VERSION", "main"), # Version "conf_py_path": "/docs/", # Path in the checkout to the docs root }docs/create_kml_files.rst (4)
44-54: Consider making the year configurable.The code currently hardcodes the year 2020. Consider making this a variable at the start of the example to make it easier for users to adapt the code for different years.
+# Configure the year for CO2 data +TARGET_YEAR = "2020" with co2_csv.open() as csvfile: reader = csv.DictReader(csvfile) for row in reader: - if row["year"] == "2020": + if row["year"] == TARGET_YEAR:
64-93: Document the height scaling factor.The code uses a multiplier of 100,000 for the CO2 emission values (
co2_emission * 100_000). Consider adding a comment explaining why this specific scaling factor was chosen and how it affects the visualization.+ # Scale CO2 emissions by 100,000 meters to make the visualization visible + # Adjust this factor based on your visualization needs geometry = force_3d(geometry, co2_emission * 100_000)
169-215: Ensure consistency in scaling factors between examples.The first example uses a scaling factor of 100,000 while this example uses 5,000. Consider:
- Using the same scaling factor for consistency
- Explaining why different scaling factors are used if intentional
- Making the scaling factor a configurable constant
Additionally, consider adding error handling for potential missing or invalid dates.
+# Configure visualization parameters +SCALING_FACTOR = 5_000 # meters per CO2 unit +START_YEAR = 1995 +END_YEAR = 2022 -for year in range(1995, 2023): +for year in range(START_YEAR, END_YEAR + 1): co2_year = co2_pa[str(year)].get(iso3_code, 0) co2_growth += co2_year kml_geometry = create_kml_geometry( - force_3d(geometry, co2_growth * 5_000), + force_3d(geometry, co2_growth * SCALING_FACTOR),
101-107: Consider more robust file path handling.While the current file path handling works, consider:
- Adding checks for file write permissions
- Using a configurable output directory
- Adding error handling for file operations
+# Configure output settings +OUTPUT_DIR = pathlib.Path("output") + +# Ensure output directory exists +OUTPUT_DIR.mkdir(exist_ok=True) + -outfile = pathlib.Path("co2_per_capita_2020.kml") +outfile = OUTPUT_DIR / f"co2_per_capita_{TARGET_YEAR}.kml" +try: with outfile.open("w") as f: f.write(kml.to_string(prettyprint=True, precision=3)) +except IOError as e: + print(f"Error writing KML file: {e}")Also applies to: 221-227
tests/hypothesis/views_test.py (2)
36-72: Consider adjusting coordinate value constraints.While the current implementation is good, there are a few potential improvements for the coordinate value ranges:
- The longitude filter
lambda x: x != 0might be too restrictive as 0° longitude (Prime Meridian) is a valid coordinate.- Similarly for latitude, filtering out 0° (Equator) might exclude valid test cases.
Consider removing these filters or documenting why these values are excluded:
- ).filter(lambda x: x != 0), + ), # Allow all values including 0 (Prime Meridian)
74-260: Excellent property-based test coverage.The test suite demonstrates good practices:
- Comprehensive testing of all view classes
- Proper validation of serialization roundtrips
- Effective use of Hypothesis strategies
- Good reuse of common test configurations
Some suggestions for enhancement:
- Consider adding edge case tests for specific KML requirements
- Document why certain values are filtered out
- Add tests for error conditions (e.g., invalid coordinate combinations)
fastkml/times.py (2)
93-114: LGTM! Consider simplifying the nested conditional expressions.The resolution and date handling logic is correct and comprehensive. However, the nested conditional expressions could be simplified for better readability.
Consider this alternative structure:
- dt = ( - dt.date() - if isinstance(dt, datetime) and resolution != DateTimeResolution.datetime - else dt - ) + if isinstance(dt, datetime) and resolution != DateTimeResolution.datetime: + dt = dt.date()
Line range hint
160-165: Consider removing arrow dependency completely.While the change to use native
dateclass is good, the method still relies onarrowfor parsing datetime strings. Consider using Python's built-indatetime.fromisoformat()for all date/time parsing to reduce dependencies.Here's a suggested implementation:
- elif len(datestr) > 10: # noqa: PLR2004 - dt = arrow.get(datestr).datetime + elif len(datestr) > 10: # noqa: PLR2004 + try: + dt = datetime.fromisoformat(datestr.replace('Z', '+00:00')) + except ValueError: + return NoneThis change would:
- Remove the dependency on the
arrowlibrary- Handle ISO format dates natively
- Properly handle UTC timezone ('Z' suffix)
- Provide better error handling
tests/data_test.py (1)
34-44: Consider enhancing test coverage with additional test cases.The test implementation is correct and verifies the basic functionality. However, consider adding more test cases to cover:
- Edge cases with whitespace in attributes
- Special characters in name/display_name
- Different data types beyond just "string"
Example enhancement:
def test_simple_field_from_string_edge_cases(self) -> None: doc = ( '<SimpleField name=" field name " type="int">' '<displayName>Special & Chars</displayName></SimpleField>' ) sf = data.SimpleField.from_string(doc) assert sf.name == "field name" # verify whitespace stripping assert sf.type_ == DataType("int") assert sf.display_name == "Special & Chars"tests/styles_test.py (1)
415-453: LGTM! Consider extracting helper methods for assertion blocks.The reordering of assertions improves readability by grouping related checks. However, the repeated assertion patterns for each style type could be extracted into helper methods to reduce duplication.
Example helper method:
def assert_icon_style(style: IconStyle, expected_id: str, expected_target_id: str) -> None: assert isinstance(style, styles.IconStyle) assert style.id == expected_id assert style.target_id == expected_target_id # ... other assertionsfastkml/features.py (1)
Line range hint
674-676: Consider makinglinkattribute required in type hintsThe class docstring states that
<Link>is required, but the type hintOptional[Link]suggests it's optional. Consider updating the type hint to reflect that this is a required attribute.- link: Optional[Link] + link: Linkfastkml/helpers.py (1)
648-650: LGTM! Consider combining the conditions.The changes improve error handling by explicitly checking for None values and ensuring type safety. However, the conditions could be combined for better readability.
Consider this minor improvement:
- if node is None or node.text is None: - return {} - assert isinstance(node.text, str) # noqa: S101 + if node is None or not isinstance(node.text, str): + return {}This change:
- Combines the None check and type check into a single condition
- Eliminates the need for a separate assertion
- Maintains the same behavior and safety guarantees
tests/repr_eq_test.py (4)
Line range hint
32-1024: Consider moving test data to a separate fixture file.The test data is quite extensive and makes the test file harder to read and maintain. Consider:
- Moving the
clean_docdata to a separate fixture file (e.g.,tests/fixtures/kml_test_data.py)- Loading the test data from the fixture file in the test class
Example refactor:
# tests/fixtures/kml_test_data.py from typing import Final import fastkml # ... other imports CLEAN_DOC: Final = fastkml.kml.KML(...) # tests/repr_eq_test.py from tests.fixtures.kml_test_data import CLEAN_DOC class TestRepr(StdLibrary): clean_doc: Final = CLEAN_DOC
Line range hint
1025-1037: Replace print statements with proper test assertions indiff_compare.The
diff_comparemethod uses print statements which is not ideal for testing. Consider:
- Returning the differences as a string or list
- Using proper test assertions to verify the differences
Example refactor:
def diff_compare(self, a: str, b: str) -> list[str]: """Compare two strings and return the differences. Args: a: First string to compare b: Second string to compare Returns: List of differences between the strings """ differences = [] differ = difflib.Differ() - for line, d in enumerate(differ.compare(a.split(), b.split())): - if d[0] in ("+", "-"): - print(line, d) + for line, diff in enumerate(differ.compare(a.split(), b.split())): + if diff[0] in ("+", "-"): + differences.append(f"{line}: {diff}") for i, chunk in enumerate(zip(wrap(a, 100), wrap(b, 100))): if chunk[0] != chunk[1]: - print(i * 100) - print(chunk[0]) - print(chunk[1]) + differences.extend([ + f"Position {i * 100}:", + f"Expected: {chunk[0]}", + f"Actual: {chunk[1]}" + ]) + return differences
Line range hint
1038-1042: Security concern: Using eval() for object reconstruction.The
test_reprmethod useseval()which can be dangerous if the input is not trusted. Consider usingast.literal_eval()which is safer for evaluating string literals.Apply this diff:
+ from ast import literal_eval def test_repr(self) -> None: """Test the __repr__ method.""" - new_doc = eval(repr(self.clean_doc), {}, eval_locals) + new_doc = literal_eval(repr(self.clean_doc))
Line range hint
1043-1051: Enhance test method docstrings with more details.The test method docstrings could provide more information about:
- Test scenarios being covered
- Expected behavior
- Edge cases if any
Example improvement:
def test_eq_str_round_trip(self) -> None: - """Test the equality of the original and the round-tripped document.""" + """Test the equality of the original and round-tripped KML document. + + This test verifies that: + 1. Converting KML to string and back preserves the document structure + 2. String representations match between original and round-tripped documents + 3. Repr representations match between original and round-tripped documents + + Note: Strict equality between objects is not guaranteed due to precision differences + """tests/hypothesis/strategies.py (2)
88-90: Simplify Nestedst.one_ofinresolutionParameterIn the
kml_datetimesstrategy, theresolutionparameter uses nestedst.one_ofcalls, which is redundant and unnecessary.Simplify the code by removing the redundant nesting:
- resolution=st.one_of( - st.none(), - st.one_of(st.none(), st.sampled_from(fastkml.enums.DateTimeResolution)), - ), + resolution=st.one_of( + st.none(), + st.sampled_from(fastkml.enums.DateTimeResolution), + ),
143-143: Specify Non-Negative Values forscaleandwidthin StylesIn the
stylesstrategy, thescaleparameter forLabelStyleand thewidthparameter forLineStylecan produce negative values, which may not be valid in the context of style properties.Set
min_valueto ensure only non-negative floats are generated:For
scaleinLabelStyle(line 143):- scale=st.floats(allow_nan=False, allow_infinity=False), + scale=st.floats(allow_nan=False, allow_infinity=False, min_value=0.0),For
widthinLineStyle(line 149):- width=st.floats(allow_nan=False, allow_infinity=False, min_value=0), + width=st.floats(allow_nan=False, allow_infinity=False, min_value=0.0),Also applies to: 149-149
fastkml/data.py (6)
119-121: Consider simplifying attribute initialization in__init__In the
__init__method ofSimpleField, the assignments toself.name,self.type_, andself.display_nameinvolve repetitive conditional statements that can be simplified for better readability. Consider using a helper function to normalize these attributes.For example:
def _normalize(value: Optional[str]) -> Optional[str]: return value.strip() or None if value else None self.name = _normalize(name) self.type_ = type_ or None self.display_name = _normalize(display_name)
Line range hint
158-181: Ensure consistent ordering ofns_idsin registry registrationsThe
ns_idstuples in theregistry.registercalls forSimpleFieldhave inconsistent orders. At line 158,ns_ids=("kml", "")is used, while at line 181,ns_ids=("", "kml")is used. For consistency and to prevent potential issues with namespace resolution, consider standardizing the order ofns_idsacross all registrations.
241-251: Refactor repeated attribute initialization inSchema.__init__Similar to
SimpleField, the__init__method ofSchemacontains repetitive code for initializingself.name,self.fields, andself.idwith conditional stripping. Refactoring this logic using a helper function can improve maintainability and reduce code duplication.For example:
self.name = _normalize(name) self.id = _normalize(id) self.fields = list(fields) if fields else []
367-369: Simplify attribute assignments inData.__init__In the
__init__method ofData, the assignments toself.name,self.value, andself.display_nameuse repetitive conditional stripping. To enhance readability, consider applying a helper function as suggested previously.
474-475: Refactor attribute initialization inSimpleDataThe initialization of
self.nameandself.valueinSimpleData.__init__follows the same pattern of conditional stripping. Refactoring with a helper function promotes code reuse and consistency across classes.
582-582: Simplifyschema_urlassignment inSchemaData.__init__The assignment to
self.schema_urlinvolves conditional stripping similar to previous classes. Using a common helper function can simplify the code and maintain consistency.For example:
self.schema_url = _normalize(schema_url) self.data = list(data) if data else []tests/hypothesis/style_test.py (1)
340-342: Enableassert_str_roundtrip_verboseintest_fuzz_styleThe
assert_str_roundtrip_verboseassertion is disabled because ofIconStyle. To achieve complete test coverage, consider resolving the issue that causes the failure and re-enable the assertion.fastkml/styles.py (2)
350-350: Consider refining the__bool__method inHotSpot.The
__bool__method in theHotSpotclass always returnsTrue, regardless of the state of its attributes. This might lead to unexpected behavior when checking the truthiness of aHotSpotinstance that doesn't have meaningful values forx,y,xunits, andyunits.Consider modifying the
__bool__method to reflect whether theHotSpotinstance has valid coordinate values:def __bool__(self) -> bool: - return True + return all([ + self.x is not None, + self.y is not None, + self.xunits is not None, + self.yunits is not None, + ])
Line range hint
425-486: Handleicon_hrefandiconparameters more robustly inIconStyle.The constructor of
IconStylenow accepts bothiconandicon_hrefparameters. There are a few considerations:
- If both
iconandicon_hrefare provided, a warning is logged, but theicon_hrefis ignored. This could be unexpected for users.- The
icon_hrefis stripped and set toNoneif empty, but this operation might raise an exception ificon_hrefis not a string.To improve robustness, consider the following changes:
- Ensure that
icon_hrefis safely stripped only if it's a non-Nonestring.- Raise an exception or provide clearer feedback when both
iconandicon_hrefare provided, instead of just logging a warning.Apply this diff to address the issues:
icon_href = icon_href.strip() or None if icon_href else None if icon_href and icon: - logger.warning( - "Both icon_href and icon were provided. " - "Ignoring icon_href and using icon.", - ) + raise ValueError("Cannot specify both 'icon' and 'icon_href'. Please provide only one.") if icon_href and not icon: icon = Icon(ns=ns, name_spaces=name_spaces, href=icon_href)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (38)
.github/workflows/run-all-tests.yml(1 hunks).github/workflows/sonarcloud.yml(1 hunks).pre-commit-config.yaml(2 hunks)README.rst(2 hunks)docs/conf.py(1 hunks)docs/configuration.rst(1 hunks)docs/create_kml_files.rst(3 hunks)docs/fastkml.rst(1 hunks)docs/modules.rst(0 hunks)docs/working_with_kml.rst(7 hunks)fastkml/about.py(1 hunks)fastkml/atom.py(2 hunks)fastkml/data.py(21 hunks)fastkml/features.py(6 hunks)fastkml/helpers.py(1 hunks)fastkml/links.py(2 hunks)fastkml/styles.py(11 hunks)fastkml/times.py(2 hunks)fastkml/utils.py(1 hunks)fastkml/validator.py(2 hunks)fastkml/views.py(7 hunks)pyproject.toml(1 hunks)sonar-project.properties(1 hunks)tests/atom_test.py(1 hunks)tests/data_test.py(4 hunks)tests/hypothesis/common.py(3 hunks)tests/hypothesis/data_test.py(1 hunks)tests/hypothesis/geometry_test.py(1 hunks)tests/hypothesis/gx_test.py(2 hunks)tests/hypothesis/multi_geometry_test.py(1 hunks)tests/hypothesis/strategies.py(2 hunks)tests/hypothesis/style_test.py(1 hunks)tests/hypothesis/times_test.py(1 hunks)tests/hypothesis/views_test.py(1 hunks)tests/repr_eq_test.py(1 hunks)tests/styles_test.py(4 hunks)tests/times_test.py(9 hunks)tests/views_test.py(0 hunks)
💤 Files with no reviewable changes (2)
- docs/modules.rst
- tests/views_test.py
✅ Files skipped from review due to trivial changes (4)
- docs/configuration.rst
- docs/fastkml.rst
- fastkml/about.py
- sonar-project.properties
🧰 Additional context used
🪛 GitHub Check: static-tests (3.12)
.github/workflows/sonarcloud.yml
[warning] 1-1:
1:1 [document-start] missing document start "---"
[warning] 2-2:
2:1 [truthy] truthy value should be one of [false, true]
🔇 Additional comments (54)
.github/workflows/sonarcloud.yml (1)
8-12: LGTM! Job configuration is clean and follows best practices.
The job configuration is appropriately minimal and uses the latest Ubuntu runner.
fastkml/utils.py (1)
10-10: LGTM: __all__ correctly updated
The __all__ list is properly updated to include the new find function while maintaining alphabetical order.
.pre-commit-config.yaml (1)
5-8: LGTM! Good addition of pre-commit CI config validation.
Adding the pre-commit-ci-config hook will help ensure the pre-commit CI configuration remains valid.
tests/hypothesis/times_test.py (1)
1-37: LGTM! Well-structured file header with proper documentation.
The file header follows best practices with clear license information, comprehensive docstring, and well-organized imports.
fastkml/validator.py (2)
17-17: LGTM: Improved code organization
The added blank line enhances readability by properly separating the copyright notice from the imports section.
102-103: Approve defensive error handling with suggestions
Good addition of a fallback mechanism when parent resolution fails. This ensures that error logging always has valid XML content to display.
Consider adding debug logging when this fallback is triggered to help diagnose XPath evaluation failures:
if parent is None:
+ logger.debug("Failed to resolve parent element via XPath, using root element as fallback")
parent = elementLet's verify test coverage for this scenario:
tests/hypothesis/common.py (2)
35-41: LGTM! New enum imports are properly organized.
The new imports from fastkml.enums are well-organized and maintain consistency with the existing import style.
67-71: LGTM! Verify test coverage for new enum types.
The new enum types are properly added to eval_locals for XML object evaluation.
Let's verify that these new enum types are being tested:
✅ Verification successful
Let me check the actual test coverage by examining the usage of these enums in the test files.
Test coverage for new enum types is comprehensive ✓
The enums are well-tested across multiple test files:
ColorMode: Used in property-based tests for IconStyle, LineStyle, PolyStyle, and LabelStyleUnits: Tested in HotSpot component with both x and y unit parametersDisplayMode: Covered in BalloonStyle testsPairKey: Used in Pair and StyleMap testing with both normal and highlight casesDataType: Tested in SimpleField data structure tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new enum types are used in property-based tests
# Expected: Find test cases using these enums in hypothesis test files
echo "Checking test coverage for new enum types..."
rg -l "ColorMode|DataType|DisplayMode|PairKey|Units" "tests/hypothesis/"
Length of output: 290
Script:
#!/bin/bash
# Check how these enums are used in the test files
echo "=== strategies.py ==="
rg -A 5 "ColorMode|DataType|DisplayMode|PairKey|Units" "tests/hypothesis/strategies.py"
echo -e "\n=== style_test.py ==="
rg -A 5 "ColorMode|DataType|DisplayMode|PairKey|Units" "tests/hypothesis/style_test.py"
echo -e "\n=== data_test.py ==="
rg -A 5 "ColorMode|DataType|DisplayMode|PairKey|Units" "tests/hypothesis/data_test.py"
Length of output: 8341
README.rst (2)
31-31: LGTM! Security improvement in coverage badge URL.
The change from HTTP to HTTPS improves security for badge requests.
111-113: Verify the pre-release installation instructions.
The installation instructions use the --pre flag, which is consistent with this being a beta release (1.0.0b2). However, we should ensure this is intentional and documented.
✅ Verification successful
Let me gather more information about the version and release status.
Let me check one more thing to verify the development status and version.
Pre-release installation instructions are correct and well-documented
The --pre flag in the installation instructions is intentional and correctly documented as this is indeed a beta release (version 1.0.0b2). This is confirmed by:
- Version 1.0.0b2 is explicitly set in
fastkml/about.py - Project is classified as "Development Status :: 4 - Beta" in pyproject.toml
- Installation instructions in README.rst appropriately use the
--preflag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this is indeed a pre-release version
rg -l "version.*b\d+" setup.py setup.cfg pyproject.toml
# Verify if pre-release installation is documented elsewhere
rg --type rst --type md -l "pip install.*--pre.*fastkml"
Length of output: 226
Script:
#!/bin/bash
# Check for version information in pyproject.toml
fd pyproject.toml --exec cat {}
# Look for version information in any file
rg -l "version.*=.*" --type python
# Check the content of README.rst for installation instructions context
rg -A 5 -B 5 "pip install.*--pre" README.rst
Length of output: 5071
Script:
#!/bin/bash
# Check the version in the about file
fd "about.py" --exec cat {}
# Look for version string in any Python file
rg "__version__.*="
Length of output: 1019
tests/hypothesis/gx_test.py (2)
36-36: LGTM! Good refactoring of datetime strategy imports.
Moving to a centralized datetime strategy improves test maintainability and consistency.
41-45: Verify float value restrictions for Angle parameters.
While excluding NaN and infinity is good, consider if there should be additional bounds for heading, roll, and tilt values based on their semantic meaning in KML.
✅ Verification successful
No additional bounds needed for Angle parameters
Based on the code examination, the current float restrictions (no NaN/infinity) are sufficient for the Angle class. The implementation follows Google's KML specification where:
- Heading, tilt, and roll are specified in degrees
- Default values are 0.0 for all parameters
- No explicit bounds are enforced in the KML spec or implementation
- The class is used primarily for track visualization in Google Earth
The test cases show various valid angle values being used, including:
- Default angles (0.0, 0.0, 0.0)
- Decimal angles (45.54676, 66.2342, 77.0)
- Simple angles (1.0, 2.0, 3.0)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Angle class implementation and usage to verify value constraints
ast-grep --pattern 'class Angle {
$$$
}'
# Search for any validation or bounds checking
rg -A 5 "heading|roll|tilt" --type python
Length of output: 119
Script:
#!/bin/bash
# Search for Angle class implementation
rg -t py "class Angle"
# Search for Angle class usage and validation
rg -t py -A 5 "heading.*roll.*tilt|roll.*tilt.*heading|tilt.*heading.*roll"
# Search for any validation or bounds checking related to these parameters
rg -t py -A 5 "(heading|roll|tilt).*(range|bound|valid|check|assert|raise)"
Length of output: 207
Script:
#!/bin/bash
# Let's examine the Angle class implementation
cat fastkml/gx.py
# Look for any test cases or usage examples
rg -t py "Angle\(" -A 3
Length of output: 20839
pyproject.toml (2)
182-186: LGTM: RST documentation validation configuration is appropriate.
The rstcheck configuration is well-structured with reasonable settings for documentation validation.
Line range hint 188-267: LGTM: Comprehensive Ruff linting configuration with appropriate rule selections.
The Ruff configuration is well-structured with:
- Appropriate base settings for auto-fixing and Python version targeting
- Comprehensive rule selection with reasonable exclusions
- Well-organized per-file ignore rules for different contexts
Let's verify the Python version compatibility:
✅ Verification successful
Ruff configuration's Python version targeting is correctly aligned with project requirements
The verification confirms that:
- Project requires Python >= 3.8
- Ruff's target-version is set to "py38"
- Both settings are in perfect alignment, ensuring consistent linting behavior across all supported Python versions (3.8 through 3.14)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the target Python version matches supported versions
# Test: Check project's Python version requirements
echo "Project's Python requirements:"
rg "requires-python|Programming Language :: Python :: \d" pyproject.toml
# Test: Check if target version matches minimum supported version
echo -e "\nRuff target version:"
rg "target-version" pyproject.toml
Length of output: 661
.github/workflows/run-all-tests.yml (2)
Line range hint 1-190: Well-structured workflow with proper security controls.
The workflow demonstrates several good practices:
- Clear job dependencies and stages
- Proper environment protection for releases
- Minimal permissions using
permissionsblock - Secure handling of credentials
Line range hint 9-11: Consider harmonizing Python version matrices across jobs.
Different jobs test against different Python version ranges:
cpython: 3.8 to 3.14-devcpython-lxml: 3.8 to 3.13doctest-lxml: only 3.12static-tests: only 3.12
While it makes sense to limit some jobs to specific versions, consider documenting the rationale for these differences.
Also applies to: 27-29, 46-48, 89-91
tests/atom_test.py (1)
155-156: LGTM! Test coverage looks complete.
The test properly validates both the internal state (name is None) and the serialization behavior (no atom:name element). This ensures that None values are handled correctly in both directions.
Let's verify the test coverage for name handling:
✅ Verification successful
Test coverage for name handling is comprehensive
The test coverage for name handling is complete with complementary test cases:
test_atom_contributor_read_no_name: Verifies deserialization when name is not presenttest_atom_contributor_no_name: Verifies both object state and serialization when name is None
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for name attribute handling
# Test: Search for all name-related assertions in atom tests
rg -A 2 "name.*None|None.*name" tests/atom_test.py
# Test: Search for XML element assertions
rg "atom:name.*not in" tests/atom_test.py
Length of output: 656
tests/hypothesis/data_test.py (1)
1-214: Well-structured property-based tests!
The implementation provides thorough testing of fastkml data structures using Hypothesis. The property-based testing approach will help catch edge cases and ensure robustness. The test methods are well-organized and cover all important aspects of the data classes.
docs/working_with_kml.rst (5)
32-35: LGTM! Clear and helpful documentation.
The explanation about finding Points, including those within MultiGeometries, is well-written and provides valuable information to users.
36-37: LGTM! Well-structured code example.
The code example effectively demonstrates the Points search functionality with clear output, and it's properly formatted as a doctest.
84-85: LGTM! Good verification example.
The addition helps users verify they found the correct Placemark by showing its name property.
167-168: LGTM! Clear transformation explanation.
The documentation clearly explains the process of transforming CascadingStyle elements for Google Earth Pro compatibility.
178-182: LGTM! Well-documented cleanup process.
The documentation clearly shows how to clean up after the transformation, with a logical flow between explanation and code example.
docs/create_kml_files.rst (1)
18-31: LGTM! Well-organized imports.
The imports are clearly structured and properly formatted in the interactive Python console style, making it easy for users to follow along.
tests/hypothesis/views_test.py (2)
1-35: LGTM! Well-structured file header and imports.
The file follows best practices with proper licensing, clear documentation, and well-organized imports.
1-260: Verify complete coverage of view classes.
Let's ensure all view-related classes from fastkml are covered by these tests.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence of the test coverage:
All view classes are properly covered by property-based tests ✓
The verification shows perfect 1:1 mapping between view classes and their corresponding test coverage:
Camera→test_fuzz_cameraLookAt→test_fuzz_look_atLatLonAltBox→test_fuzz_lat_lon_alt_boxLod→test_fuzz_lodRegion→test_fuzz_region
The only uncovered class is _AbstractView which is correct as it's a private abstract base class not meant to be tested directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all view-related classes are covered by tests
# Find all view-related classes in fastkml
echo "View-related classes in fastkml:"
rg "class.*\(" fastkml/views.py
# Find corresponding test coverage
echo -e "\nTest coverage in views_test.py:"
rg "test_fuzz.*\(" tests/hypothesis/views_test.py
Length of output: 1579
fastkml/atom.py (2)
288-290: Simplify the conditional expressions and update docstring.
The current implementation can be made clearer. Also, the docstring should document the handling of empty strings.
Consider this simpler implementation:
- self.name = name.strip() or None if name else None
- self.uri = uri.strip() or None if uri else None
- self.email = email.strip() or None if email else None
+ self.name = name and name.strip() or None
+ self.uri = uri and uri.strip() or None
+ self.email = email and email.strip() or NoneAlso, please update the class docstring to mention that empty strings are converted to None:
"""
Represents a person, corporation, or similar entity.
Attributes
----------
name (Optional[str]): A human-readable name for the person. Empty strings are converted to None.
uri (Optional[str]): A home page for the person. Empty strings are converted to None.
email (Optional[str]): An email address for the person. Empty strings are converted to None.
"""Let's verify if this behavior change might affect existing code:
#!/bin/bash
# Search for tests or code that might rely on empty strings
rg -B 2 -A 2 'assertEqual.*"".*name'
rg -B 2 -A 2 'assertEqual.*"".*uri'
rg -B 2 -A 2 'assertEqual.*"".*email'261-263: LGTM! Consider updating registry entries.
The type annotations correctly reflect that these attributes are now optional. However, the registry entries for these attributes still specify classes=(str,). Consider updating them to handle None values consistently.
Let's verify the registry handling of None values:
tests/data_test.py (1)
84-86: LGTM! Verify complete migration of type to type_.
The attribute access updates from type to type_ are correct and consistent with Python naming conventions.
Let's verify that all occurrences have been updated:
Also applies to: 95-95
✅ Verification successful
Let me search for any remaining instances of .type attribute access in the codebase to ensure a complete verification.
Migration from type to type_ is correctly implemented
The codebase scan reveals that the remaining .type occurrences are either:
- Module imports (e.g.,
from pygeoif.types import ...) - Part of the
atom.Linkclass wheretypeis intentionally used as it represents the MIME type attribute of an Atom link
The changes in the test file correctly use type_ for schema field type attributes, and no instances of the old .type attribute access were found in the schema-related code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old 'type' attribute
# Expected: No occurrences of SimpleField(...).type or field.type access
# Search for potential missed occurrences
rg "SimpleField.*\.type[^_]" || echo "No remaining .type occurrences found"
Length of output: 113
Script:
#!/bin/bash
# Search for any field.type attribute access in Python files
rg "\.type[^_]" --type py -A 2
# Also check for type attribute in class/field definitions
ast-grep --pattern 'class $_ {
$$$
type
$$$
}'
Length of output: 2251
tests/times_test.py (4)
105-105: LGTM: Consistent updates to date assertions
The changes consistently update the date parsing tests to expect datetime.date objects instead of datetime.datetime objects, which aligns with the simplified date handling approach. The test coverage remains comprehensive across different date formats.
Also applies to: 119-119, 126-126, 141-141, 148-148
298-298: LGTM: Consistent timestamp parsing updates
The timestamp reading tests have been appropriately updated to expect datetime.date objects for date-only formats while maintaining datetime.datetime expectations for full timestamps. This change maintains proper type distinction based on the input format.
Also applies to: 311-311, 324-324, 337-337
396-396: LGTM: Proper handling of dates in complex KML structures
The updates to timespan and feature tests maintain proper type handling, using datetime.date for date-only values while preserving datetime.datetime for timestamp values. This ensures accurate representation of temporal data in complex KML structures.
Also applies to: 418-418
Line range hint 1-424: Verify test coverage for date handling changes
The changes consistently update the tests to handle date-only values appropriately. Let's verify the test coverage for the updated date handling functionality.
✅ Verification successful
Test coverage is comprehensive for date handling functionality
The test coverage for date handling is complete and well-structured:
- The test file covers all date/time formats supported by
KmlDateTimeclass:- Year only (YYYY)
- Year-month (YYYY-MM)
- Full date (YYYY-MM-DD)
- Full datetime with timezone
- UTC and offset timezones
- All edge cases are tested:
- Invalid dates/months
- Empty strings
- None values
- Different resolution handling
- The example file
shp2kml_timed.pydemonstrates practical usage with both date-only and datetime values - The implementation in
fastkml/times.pyaligns perfectly with the test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for date handling
# Look for any untested date parsing scenarios
rg -l "KmlDateTime.parse|datetime.date" --type py --glob '!tests/*'
# Check for any TODO comments related to date handling
rg -i "todo.*\b(date|time)\b" --type py
Length of output: 137
Script:
#!/bin/bash
# Check the implementation of KmlDateTime and its usage in the codebase
ast-grep --pattern 'class KmlDateTime:
$$$'
# Check the example file that uses KmlDateTime
cat examples/shp2kml_timed.py
# Look for actual date/time parsing implementation
rg -p "def parse" --type py fastkml/times.py
Length of output: 10164
tests/hypothesis/geometry_test.py (1)
72-72: LGTM! Enhanced test coverage for altitude modes.
The change to use AltitudeMode directly in st.sampled_from improves test coverage by including all possible altitude mode values. This ensures better validation of the roundtrip serialization for all supported altitude modes.
Let's verify that all altitude modes are properly defined and documented:
✅ Verification successful
Let me gather more information about the AltitudeMode enum values and its documentation.
Let me get the complete enum values since the previous output was truncated.
Using AltitudeMode in sampled_from is correct and well-supported
The verification confirms that:
- AltitudeMode is a properly defined enum with three well-documented values: clampToGround (default), relativeToGround, and absolute
- The enum is extensively used across the test suite, including property-based tests
- The documentation in docs/create_kml_files.rst provides usage examples
- The test coverage is comprehensive, with tests verifying serialization, roundtrip conversions, and edge cases
The change to use AltitudeMode directly in st.sampled_from ensures all valid altitude modes are tested, making the property-based tests more robust.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AltitudeMode enum definition and usage
# Expected: Find the enum definition and its values
# Find the AltitudeMode enum definition
echo "=== AltitudeMode Definition ==="
rg -A 5 "class AltitudeMode.*Enum"
# Find where altitude modes are documented
echo -e "\n=== AltitudeMode Documentation ==="
rg -l "AltitudeMode" docs/
Length of output: 551
Script:
#!/bin/bash
# Get the complete AltitudeMode enum definition and values
echo "=== Complete AltitudeMode Definition ==="
rg -A 15 "class AltitudeMode.*Enum" fastkml/enums.py
# Check the documentation content
echo -e "\n=== AltitudeMode Documentation Content ==="
cat docs/create_kml_files.rst | rg -A 10 "AltitudeMode"
# Check actual usage in tests
echo -e "\n=== AltitudeMode Usage in Tests ==="
rg "AltitudeMode" tests/
Length of output: 10795
Script:
#!/bin/bash
# Get the complete enum definition using a larger context
echo "=== Complete AltitudeMode Definition ==="
rg -A 25 "class AltitudeMode.*Enum" fastkml/enums.py
# Check the base RelaxedEnum implementation
echo -e "\n=== RelaxedEnum Base Class ==="
rg -B 5 "class RelaxedEnum" fastkml/enums.py
Length of output: 1860
tests/hypothesis/multi_geometry_test.py (2)
Line range hint 1-600: Excellent test design and coverage!
The test file demonstrates several good practices:
- Comprehensive property-based testing of geometry types
- Well-structured helper functions
- Strong type hints and validation
68-68: LGTM! More maintainable sampling strategy.
The change to sample directly from AltitudeMode enum is an improvement as it:
- Automatically includes all enum values
- Is more maintainable and future-proof
- Aligns with property-based testing principles
Let's verify that all AltitudeMode values are valid for the tested geometry types:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Sampling from AltitudeMode enum is safe and comprehensive
The change to use st.sampled_from(AltitudeMode) is verified to be correct because:
- The AltitudeMode enum includes all valid KML altitude modes: clampToGround (default), relativeToGround, and absolute
- The test strategy correctly handles both None and all enum values through
st.one_of(st.none(), st.sampled_from(AltitudeMode)) - The test file includes proper validation of altitude mode behavior in geometry serialization/deserialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AltitudeMode enum usage and values
# Search for AltitudeMode definition and its usage in geometry-related code
# Find AltitudeMode enum definition
echo "=== AltitudeMode Definition ==="
rg -A 5 "class AltitudeMode"
# Find geometry files that use AltitudeMode
echo -e "\n=== AltitudeMode Usage in Geometry ==="
rg "AltitudeMode" --type python
Length of output: 563
Script:
#!/bin/bash
# Let's try a different approach to find AltitudeMode definition and usage
# Find AltitudeMode enum definition with more context
echo "=== AltitudeMode Definition ==="
rg -A 10 "class AltitudeMode" fastkml/enums.py
# Find all files using AltitudeMode
echo -e "\n=== Files Using AltitudeMode ==="
rg --files-with-matches "AltitudeMode"
# Show actual usage contexts
echo -e "\n=== AltitudeMode Usage Contexts ==="
rg "AltitudeMode" -B 2 -A 2
Length of output: 50059
Script:
#!/bin/bash
# Let's check the AltitudeMode enum definition and values
echo "=== AltitudeMode Enum Definition ==="
rg -A 15 "class AltitudeMode" fastkml/enums.py
# Let's also check how it's used in the test file
echo -e "\n=== AltitudeMode Usage in Test File ==="
rg -B 2 -A 2 "AltitudeMode" tests/hypothesis/multi_geometry_test.py
Length of output: 5065
Script:
#!/bin/bash
# Let's get the complete AltitudeMode enum definition to see all possible values
echo "=== Complete AltitudeMode Definition ==="
cat fastkml/enums.py | awk '/class AltitudeMode/,/^$/ {print}'
# Also check the test strategy implementation
echo -e "\n=== Test Strategy Implementation ==="
cat tests/hypothesis/multi_geometry_test.py | awk '/def common_geometry/,/^$/ {print}'
Length of output: 427
Script:
#!/bin/bash
# Let's try a different approach to get the AltitudeMode enum values
echo "=== AltitudeMode Enum Values ==="
rg -A 20 "class AltitudeMode" --no-heading fastkml/enums.py
# And get the test strategy context
echo -e "\n=== Test Strategy Context ==="
rg -B 5 -A 5 "st.sampled_from\(AltitudeMode\)" tests/hypothesis/multi_geometry_test.py
Length of output: 1704
tests/styles_test.py (2)
18-18: LGTM!
Added a blank line after imports to follow PEP 8 style guidelines.
456-456: LGTM! Appropriate use of noqa and assertion refinement.
The PLR0915 noqa comment is justified for this comprehensive test method, and the styleUrl assertion modification maintains test coverage while being more precise.
Also applies to: 515-515
fastkml/views.py (2)
37-38: LGTM! Clean export list.
The __all__ list correctly exports only the public classes, following Python's best practices for module exports.
346-357: LGTM! Consistent altitude mode handling across classes.
The registration of altitude_mode with both kml and gx namespaces is consistent across Camera, LookAt, and LatLonAltBox classes. This properly supports Google Earth extensions while maintaining backward compatibility.
Let's verify the consistency of altitude mode usage across the codebase:
Also applies to: 460-471, 642-642
✅ Verification successful
LGTM! Altitude mode registration is consistent across classes
The altitude mode registration is correctly implemented with consistent namespace support (kml, gx) and default values across Camera, LookAt, and LatLonAltBox classes. The code shows:
-
All three classes use the same registry pattern with identical configuration:
- Same namespace IDs:
("kml", "gx", "") - Same node name:
altitudeMode - Same default:
AltitudeMode.clamp_to_ground - Same helper functions:
subelement_enum_kwarg,enum_subelement
- Same namespace IDs:
-
The implementation properly supports both KML standard modes and Google Earth extensions through the
AltitudeModeenum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify altitude mode usage consistency
# Look for any other altitude mode registrations that might need similar updates
# Search for altitude mode registrations
rg -l "altitude_mode.*node_name.*altitudeMode" | \
while read -r file; do
echo "=== $file ==="
rg -A 5 "altitude_mode.*node_name.*altitudeMode" "$file"
done
Length of output: 63
Script:
#!/bin/bash
# Let's try a different approach to find altitude mode registrations
# First, let's search for registry entries related to altitude mode
rg -i "altitudemode" -B2 -A5
# Also search for any other potential altitude mode related code
rg -i "altitude_mode" -B2 -A5
Length of output: 246358
fastkml/features.py (4)
34-34: LGTM!
The new import is correctly placed and follows the project's import organization pattern.
101-102: LGTM!
The addition of _default_nsid properly sets the KML namespace for the Snippet class.
136-136: LGTM!
The improved text handling ensures consistent behavior by stripping whitespace and properly handling empty strings.
290-290: LGTM!
The description handling is now consistent with the Snippet class's text handling, ensuring uniform behavior across the codebase.
tests/repr_eq_test.py (2)
Line range hint 1-18: LGTM! Well-organized imports and file structure.
The file follows good practices with:
- Clear copyright header and module docstring
- Properly organized imports (stdlib, third-party, local)
- Type hints usage
Line range hint 1052-1054: LGTM! Good test class structure.
The inheritance hierarchy allows for testing both standard library and lxml parsers by reusing the test methods.
fastkml/data.py (1)
682-682: Verify element filtering in ExtendedData.__init__
The list comprehension [e for e in elements if e] filters out falsy elements from elements. Verify that this behavior is intended and that no valid Data or SchemaData instances are excluded inadvertently.
If the goal is to exclude only None values, consider explicitly checking for None to avoid unintended exclusion of valid but falsy elements (e.g., empty strings or zero values).
tests/hypothesis/style_test.py (3)
40-55: Test method test_fuzz_style_url looks good
The test effectively checks the StyleUrl class with optional URL inputs using Hypothesis strategies.
56-80: Proper testing of HotSpot with optional parameters
The test_fuzz_hot_spot method accurately tests the HotSpot class by considering various combinations of optional floats and units.
475-528: test_fuzz_style_map_pairs method is comprehensive
The method effectively tests StyleMap with both normal and highlight pairs, ensuring that the StyleMap behaves correctly with valid key combinations.
fastkml/styles.py (4)
1287-1287:
Correct the node_name for the Pair class registry.
The node_name for the Pair class has been updated to "Style", but in KML, the Pair element should contain either a styleUrl or a Style object.
Consider that changing the node_name to "Style" might cause issues when parsing or serializing KML files. Ensure that both styleUrl and Style are correctly handled.
registry.register(
Pair,
RegistryItem(
ns_ids=("kml",),
attr_name="style",
- node_name="Style",
+ node_name=None, # Adjust accordingly to handle both 'styleUrl' and 'Style'
classes=(
StyleUrl,
Style,
),
get_kwarg=xml_subelement_kwarg,
set_element=xml_subelement,
),
)Please verify the handling of styleUrl and Style within Pair elements to ensure compatibility with KML specifications.
Line range hint 65-77: Verify the impact of removing id and target_id from StyleUrl.
The StyleUrl class no longer accepts id and target_id parameters in its constructor. This change may affect any code that instantiates StyleUrl with these parameters or relies on these attributes.
Run the following script to identify any instantiations of StyleUrl that use id or target_id:
#!/bin/bash
# Description: Find all instances where `StyleUrl` is instantiated with `id` or `target_id`.
# Search for `StyleUrl(` with `id=` or `target_id=` arguments.
rg --python -A 2 $'StyleUrl\\(.*(id=|target_id=)'520-524:
Avoid potential recursion in icon_href property.
The icon_href property in IconStyle might lead to a recursion error if self.icon is not properly managed.
Ensure that the Icon class's href attribute is accessed safely. Additionally, if self.icon.href might not exist, handle that scenario:
@property
def icon_href(self) -> Optional[str]:
"""Return the icon href."""
- return self.icon.href if self.icon else None
+ return getattr(self.icon, 'href', None)Likely invalid or redundant comment.
1172-1172: Include all relevant style classes in Style registry.
In the registry.register call for Style, ensure that all applicable style classes are included, especially if new styles have been introduced or modified.
Run the following script to confirm that all style classes are registered correctly:
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis | ||
| - name: SonarCloud Scan | ||
| uses: SonarSource/sonarcloud-github-action@master | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any | ||
| SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} |
There was a problem hiding this comment.
💡 Codebase verification
Consider updating SonarCloud action to a stable version
The configuration is mostly well set up with a proper sonar-project.properties file in place. However, you should update the SonarCloud action from @master to the latest stable version @v3.1.0 to ensure build reproducibility and stability.
- uses: SonarSource/[email protected]🔗 Analysis chain
Verify SonarCloud configuration and consider action versioning.
While the basic setup is correct, there are a few important points to address:
- Consider using a specific version tag instead of
@masterfor the SonarCloud action to ensure reproducible builds. - Ensure the
SONAR_TOKENsecret is configured in your repository settings.
Let's verify if the required configuration file exists:
Would you like assistance in:
- Creating a sonar-project.properties file with recommended settings?
- Updating the workflow to use a specific version of the SonarCloud action?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for sonar-project.properties file
if ! [ -f "sonar-project.properties" ]; then
echo "Warning: sonar-project.properties file is missing"
echo "This file is required for SonarCloud configuration"
fi
Length of output: 37
Script:
#!/bin/bash
# Let's check for sonar configuration in multiple possible locations
echo "Checking for sonar configuration files:"
find . -maxdepth 2 -type f -name "sonar-project.properties" -o -name ".sonarcloud.properties"
# Let's also check if there's any sonar configuration in package.json
echo -e "\nChecking for sonar configuration in package.json:"
if [ -f "package.json" ]; then
grep -A 5 "sonar" package.json
fi
# Check for any yaml files that might contain sonar configuration
echo -e "\nChecking for yaml files with sonar configuration:"
rg -l "sonar" --type yaml
Length of output: 489
Script:
#!/bin/bash
# Let's check the content of sonar-project.properties
echo "Content of sonar-project.properties:"
cat sonar-project.properties
# Let's also check the latest stable version of the SonarCloud action
echo -e "\nLatest SonarCloud action releases:"
gh release list --repo SonarSource/sonarcloud-github-action --limit 3 2>/dev/null || echo "Unable to fetch releases"
Length of output: 764
| default_language_version: | ||
| python: python3.12 |
There was a problem hiding this comment.
💡 Codebase verification
Python 3.12 default in pre-commit may be too restrictive
The project supports a wide range of Python versions (>=3.8) and actively tests against Python 3.8 through 3.14-dev in CI. Setting Python 3.12 as the default in pre-commit config could prevent contributors using older Python versions from running pre-commit hooks locally, despite the project supporting those versions.
pyproject.tomlrequires Python ">=3.8"- CI workflows test against Python versions 3.8 through 3.14-dev
- Pre-commit's default Python version (3.12) is more restrictive than the project's actual requirements
Consider using Python 3.8 as the default to match the minimum supported version, or removing the default Python version specification to let pre-commit use the system's Python version.
🔗 Analysis chain
Verify Python 3.12 compatibility across environments.
Setting Python 3.12 as the default version is forward-looking but might impact environments not yet upgraded to this relatively new Python version.
Let's check the project's Python version requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Python version specifications in project files
echo "Checking pyproject.toml for Python version requirements..."
rg "python" pyproject.toml
echo "\nChecking tox.ini for test environments..."
rg "py\d{2}" tox.ini
echo "\nChecking CI workflow files for Python versions..."
rg "python-version" .github/workflows/
Length of output: 1838
| - repo: https://github.com/astral-sh/ruff-pre-commit | ||
| rev: 'v0.7.2' | ||
| hooks: | ||
| - id: ruff | ||
| - id: ruff-format |
There was a problem hiding this comment.
💡 Codebase verification
Ruff-format configuration needs adjustment to maintain consistency
The current configuration is missing explicit format settings to match the previous style. Based on the findings:
- Previous isort config used
force_single_line = trueandline_length = 88 - No explicit ruff format settings are configured
- Black's settings are not present in pyproject.toml
To maintain consistency with the previous formatting style, you should add:
- Add
[tool.ruff.format]section to pyproject.toml with:line-length = 88to match previous settingsquote-style = "double"to match Black's defaultindent-style = "space"to maintain Python conventions
🔗 Analysis chain
Verify ruff-format configuration matches previous formatting rules.
The addition of ruff-format to replace black and isort is a good optimization. However, we should ensure the formatting rules are consistent with the project's previous style.
Let's check for any ruff configuration:
Let me gather more information about the previous black configuration to ensure formatting consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for ruff configuration
echo "Checking for ruff configuration in pyproject.toml..."
rg "\[tool\.ruff\]" -A 20 pyproject.toml
echo "\nChecking for any .ruff.toml..."
fd -g ".ruff.toml"
echo "\nChecking for previous black configuration..."
rg "black" pyproject.toml
Length of output: 871
Script:
#!/bin/bash
# Check for previous black and isort configurations
echo "Checking for black configuration in pyproject.toml..."
rg "\[tool\.black\]" -A 10 pyproject.toml
echo "\nChecking for isort configuration..."
rg "\[tool\.isort\]" -A 10 pyproject.toml
echo "\nChecking for ruff format settings..."
rg "\[tool\.ruff\.format\]" -A 10 pyproject.toml
Length of output: 672
| @given( | ||
| id=st.one_of(st.none(), nc_name()), | ||
| target_id=st.one_of(st.none(), nc_name()), | ||
| begin=st.one_of(st.none(), kml_datetimes()), | ||
| end=st.one_of(st.none(), kml_datetimes()), | ||
| ) | ||
| def test_fuzz_time_span( | ||
| self, | ||
| id: typing.Optional[str], | ||
| target_id: typing.Optional[str], | ||
| begin: typing.Optional[fastkml.times.KmlDateTime], | ||
| end: typing.Optional[fastkml.times.KmlDateTime], | ||
| ) -> None: | ||
| time_span = fastkml.TimeSpan(id=id, target_id=target_id, begin=begin, end=end) | ||
|
|
||
| assert_repr_roundtrip(time_span) | ||
| assert_str_roundtrip(time_span) | ||
| assert_str_roundtrip_terse(time_span) | ||
| assert_str_roundtrip_verbose(time_span) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for temporal order in TimeSpan tests.
The test should verify that when both begin and end are provided, they maintain a logical temporal order (begin <= end). Consider adding:
- A custom Hypothesis composite strategy to generate valid begin/end pairs
- Assertions to verify temporal consistency
Example implementation:
from hypothesis import assume
@given(
id=st.one_of(st.none(), nc_name()),
target_id=st.one_of(st.none(), nc_name()),
begin=st.one_of(st.none(), kml_datetimes()),
end=st.one_of(st.none(), kml_datetimes()),
)
def test_fuzz_time_span(
self,
id: typing.Optional[str],
target_id: typing.Optional[str],
begin: typing.Optional[fastkml.times.KmlDateTime],
end: typing.Optional[fastkml.times.KmlDateTime],
) -> None:
# Ensure temporal order when both timestamps are provided
assume(not (begin and end) or begin <= end)
time_span = fastkml.TimeSpan(id=id, target_id=target_id, begin=begin, end=end)
# Verify stored values and temporal order
assert time_span.id == id
assert time_span.target_id == target_id
assert time_span.begin == begin
assert time_span.end == end
if begin and end:
assert begin <= end
# Existing assertions
assert_repr_roundtrip(time_span)
assert_str_roundtrip(time_span)
assert_str_roundtrip_terse(time_span)
assert_str_roundtrip_verbose(time_span)| @given( | ||
| north=st.one_of( | ||
| st.none(), | ||
| st.floats(allow_nan=False, allow_infinity=False, min_value=0, max_value=90), | ||
| ), | ||
| south=st.one_of( | ||
| st.none(), | ||
| st.floats(allow_nan=False, allow_infinity=False, min_value=0, max_value=90), | ||
| ), | ||
| east=st.one_of( | ||
| st.none(), | ||
| st.floats( | ||
| allow_nan=False, | ||
| allow_infinity=False, | ||
| min_value=0, | ||
| max_value=180, | ||
| ), | ||
| ), | ||
| west=st.one_of( | ||
| st.none(), | ||
| st.floats( | ||
| allow_nan=False, | ||
| allow_infinity=False, | ||
| min_value=0, | ||
| max_value=180, | ||
| ), | ||
| ), | ||
| min_altitude=st.one_of( | ||
| st.none(), | ||
| st.floats(allow_nan=False, allow_infinity=False).filter(lambda x: x != 0), | ||
| ), | ||
| max_altitude=st.one_of( | ||
| st.none(), | ||
| st.floats(allow_nan=False, allow_infinity=False).filter(lambda x: x != 0), | ||
| ), | ||
| altitude_mode=st.one_of(st.none(), st.sampled_from(fastkml.enums.AltitudeMode)), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add coordinate validation in LatLonAltBox test.
The test should ensure that north latitude is greater than south latitude when both are provided. Consider adding a composite strategy or assumption to enforce this constraint.
Example approach using @hypothesis.assume:
@given(...)
def test_fuzz_lat_lon_alt_box(self, ...):
from hypothesis import assume
if north is not None and south is not None:
assume(north > south)
# ... rest of the test| assert_repr_roundtrip(icon_style) | ||
| assert_str_roundtrip(icon_style) | ||
| assert_str_roundtrip_terse(icon_style) | ||
| # the IconStyle does only allow for simple Icons, thats why | ||
| # assert_str_roundtrip_verbose fails at this time | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Investigate the failing assert_str_roundtrip_verbose in IconStyle tests
The comment notes that assert_str_roundtrip_verbose fails due to limitations with IconStyle. It's important to explore the cause of this failure to ensure comprehensive testing. If the issue stems from handling complex icons, consider implementing the necessary logic or documenting the limitation explicitly.
| fastkml.styles.HotSpot, | ||
| x=st.floats(allow_nan=False, allow_infinity=False).filter( | ||
| lambda x: x != 0, | ||
| ), | ||
| xunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)), | ||
| y=st.floats(allow_nan=False, allow_infinity=False).filter( | ||
| lambda x: x != 0, | ||
| ), | ||
| yunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Include zero values for x and y in HotSpot tests
The current strategy filters out zero values for x and y, excluding valid HotSpot positions at the origin. Zero is a valid value in KML for x and y. Consider removing the filters to include zero in the tests.
Apply the following changes to include zero values:
st.builds(
fastkml.styles.HotSpot,
- x=st.floats(allow_nan=False, allow_infinity=False).filter(lambda x: x != 0),
+ x=st.floats(allow_nan=False, allow_infinity=False),
xunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)),
- y=st.floats(allow_nan=False, allow_infinity=False).filter(lambda x: x != 0),
+ y=st.floats(allow_nan=False, allow_infinity=False),
yunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fastkml.styles.HotSpot, | |
| x=st.floats(allow_nan=False, allow_infinity=False).filter( | |
| lambda x: x != 0, | |
| ), | |
| xunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)), | |
| y=st.floats(allow_nan=False, allow_infinity=False).filter( | |
| lambda x: x != 0, | |
| ), | |
| yunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)), | |
| ), | |
| ), | |
| fastkml.styles.HotSpot, | |
| x=st.floats(allow_nan=False, allow_infinity=False), | |
| xunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)), | |
| y=st.floats(allow_nan=False, allow_infinity=False), | |
| yunits=st.one_of(st.none(), st.sampled_from(fastkml.enums.Units)), | |
| ), | |
| ), |
| @given( | ||
| id=st.one_of(st.none(), nc_name()), | ||
| target_id=st.one_of(st.none(), nc_name()), | ||
| pairs=st.one_of( | ||
| st.none(), | ||
| st.lists( | ||
| st.builds( | ||
| fastkml.styles.Pair, | ||
| key=st.sampled_from(fastkml.enums.PairKey), | ||
| style=st.one_of( | ||
| st.builds( | ||
| fastkml.StyleUrl, | ||
| url=urls(), | ||
| ), | ||
| st.builds( | ||
| fastkml.Style, | ||
| styles=st.lists( | ||
| styles(), | ||
| min_size=1, | ||
| max_size=1, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ) | ||
| def test_fuzz_style_map_one_pair( | ||
| self, | ||
| id: typing.Optional[str], | ||
| target_id: typing.Optional[str], | ||
| pairs: typing.Optional[typing.Tuple[fastkml.styles.Pair]], | ||
| ) -> None: | ||
| style_map = fastkml.StyleMap(id=id, target_id=target_id, pairs=pairs) | ||
|
|
||
| assert_repr_roundtrip(style_map) | ||
| assert_str_roundtrip(style_map) | ||
| assert_str_roundtrip_terse(style_map) | ||
| assert_str_roundtrip_verbose(style_map) | ||
|
|
There was a problem hiding this comment.
Ensure StyleMap contains at least one Pair
The test_fuzz_style_map_one_pair method allows pairs to be None, which contradicts the KML requirement that a StyleMap must contain at least one Pair. Modify the strategy to ensure pairs is a non-empty list of Pair instances.
Remove the possibility of pairs being None:
pairs=st.one_of(
- st.none(),
st.lists(
st.builds(
fastkml.styles.Pair,
key=st.sampled_from(fastkml.enums.PairKey),
style=st.one_of(
st.builds(
fastkml.StyleUrl,
url=urls(),
),
st.builds(
fastkml.Style,
styles=st.lists(
styles(),
min_size=1,
max_size=1,
),
),
),
),
),
),Committable suggestion skipped: line range outside the PR's diff.
| id=st.one_of(st.none(), nc_name()), | ||
| target_id=st.one_of(st.none(), nc_name()), | ||
| key=st.sampled_from(fastkml.enums.PairKey), | ||
| style=st.one_of( | ||
| st.builds( | ||
| fastkml.StyleUrl, | ||
| url=urls(), | ||
| ), | ||
| st.builds( | ||
| fastkml.Style, | ||
| styles=st.lists( | ||
| styles(), | ||
| min_size=1, | ||
| max_size=1, | ||
| ), | ||
| ), | ||
| ), | ||
| ) | ||
| def test_fuzz_pair( | ||
| self, | ||
| id: typing.Optional[str], | ||
| target_id: typing.Optional[str], | ||
| key: typing.Optional[fastkml.enums.PairKey], | ||
| style: typing.Union[fastkml.StyleUrl, fastkml.Style, None], | ||
| ) -> None: | ||
| pair = fastkml.styles.Pair(id=id, target_id=target_id, key=key, style=style) | ||
|
|
||
| assert_repr_roundtrip(pair) | ||
| assert_str_roundtrip(pair) | ||
| assert_str_roundtrip_terse(pair) | ||
| assert_str_roundtrip_verbose(pair) | ||
|
|
There was a problem hiding this comment.
Ensure key and style are not None in Pair tests
In the test_fuzz_pair method, the key and style parameters are allowed to be None. According to the KML specification, both key and style are required fields in a Pair. Adjust the strategies to prevent None values for these parameters.
Update the strategies to exclude None values:
key=st.sampled_from(fastkml.enums.PairKey),
- style=st.one_of(
+ style=st.one_of(
st.builds(
fastkml.StyleUrl,
url=urls(),
),
st.builds(
fastkml.Style,
styles=st.lists(
styles(),
min_size=1,
max_size=1,
),
),
- st.none(),
),Committable suggestion skipped: line range outside the PR's diff.
| @@ -982,7 +988,7 @@ def __init__( | |||
| ) | |||
| self.bg_color = bg_color | |||
| self.text_color = text_color | |||
| self.text = text | |||
| self.text = text.strip() or None if text else None | |||
There was a problem hiding this comment.
Handle empty or whitespace-only text in BalloonStyle.
The text attribute is being stripped and set to None if empty. However, if text is a non-string type, this could raise an exception.
Ensure that text is a string before applying strip(). Modify the code as follows:
self.text = text.strip() or None if isinstance(text, str) and text.strip() else None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.text = text.strip() or None if text else None | |
| self.text = text.strip() or None if isinstance(text, str) and text.strip() else None |
User description
PR Type
enhancement, tests, documentation, configuration changes
Description
data,views,styles,features,atom,links,times,helpers,utils) for improved XML handling, logic, and error handling.styles,views,data,times,gx,multi_geometry, andgeometry.1.0.0b2and added module exports inabout.py.Changes walkthrough 📝
13 files
style_test.py
Add property-based tests for styles module using Hypothesis.tests/hypothesis/style_test.py
StyleUrl,HotSpot,IconStyle, etc.views_test.py
Add property-based tests for views module using Hypothesis.tests/hypothesis/views_test.py
Lod,LatLonAltBox,Region,Camera, andLookAt.styles_test.py
Update style tests to align with refactored style classes.tests/styles_test.py
idandtarget_idattributes inStyleUrl.data_test.py
Add property-based tests for data module using Hypothesis.tests/hypothesis/data_test.py
SimpleField,Schema,Data,SimpleData,SchemaData, andExtendedData.strategies.py
Introduce new Hypothesis strategies for KML data types.tests/hypothesis/strategies.py
times_test.py
Update times tests to align with datetime handling changes.tests/times_test.py
KmlDateTimehandling.data_test.py
Enhance data tests with additional cases and assertions.tests/data_test.py
SimpleFieldfrom string parsing.SchemaandSimpleFieldattributes.times_test.py
Add property-based tests for times module using Hypothesis.tests/hypothesis/times_test.py
TimeStampandTimeSpanusing Hypothesis.gx_test.py
Refactor gx_test strategies for improved datetime handling.tests/hypothesis/gx_test.py
TrackItemstrategy to usekml_datetimes.track_itemsstrategy.common.py
Enhance common testing utilities with additional imports.tests/hypothesis/common.py
atom_test.py
Update atom tests to align with optional attribute changes.tests/atom_test.py
_Personattribute handling.multi_geometry_test.py
Simplify multi_geometry_test altitude_mode strategy.tests/hypothesis/multi_geometry_test.py
altitude_modestrategy to useAltitudeModeenum directly.geometry_test.py
Simplify geometry_test altitude_mode strategy.tests/hypothesis/geometry_test.py
altitude_modestrategy to useAltitudeModeenum directly.9 files
data.py
Refactor data module classes for XML handling improvements.fastkml/data.py
SimpleFieldandSchemaclasses to inherit from_XMLObject.values.
views.py
Simplify view classes by removing time primitive attribute.fastkml/views.py
time_primitiveattribute from view classes.styles.py
Refactor styles module for improved XML handling and logic.fastkml/styles.py
StyleUrlto inherit from_XMLObject.IconStyleto handleicon_hrefandiconattributes.HotSpotclass.features.py
Refactor features module for improved XML handling.fastkml/features.py
Snippetclass to inherit from_XMLObject.times.py
Improve KmlDateTime handling for date and datetime distinctions.fastkml/times.py
KmlDateTimeto handle date and datetime distinctions.atom.py
Refactor atom module for optional attribute handling.fastkml/atom.py
_Personclass to handle optional attributes.links.py
Update links module for improved href handling.fastkml/links.py
Iconclass to handlehrefattribute with whitespacestripping.
helpers.py
Enhance helpers with assertions for node text.fastkml/helpers.py
subelement_text_kwarg.utils.py
Update utils module exports to include find.fastkml/utils.py
findto module exports.1 files
repr_eq_test.py
Apply formatting changes to repr_eq_test for consistency.tests/repr_eq_test.py
1 files
validator.py
Enhance validator module with additional error handling.fastkml/validator.py
Nonevalues in XML validation.6 files
conf.py
Configure Sphinx HTML context for GitHub integration.docs/conf.py
create_kml_files.rst
Revise create_kml_files documentation with interactive examples.docs/create_kml_files.rst
working_with_kml.rst
Improve working_with_kml documentation with interactive examples.docs/working_with_kml.rst
fastkml.rst
Revise fastkml documentation structure and references.docs/fastkml.rst
validator.README.rst
Update README with coverage badge and installation instructions.README.rst
configuration.rst
Enhance configuration documentation with code block example.docs/configuration.rst
6 files
about.py
Update version and exports in about module.fastkml/about.py
1.0.0b2.__all__for module exports..pre-commit-config.yaml
Update pre-commit configuration with new hooks and versions..pre-commit-config.yaml
ruff-formatandcheck-pre-commit-ci-confighooks.sonarcloud.yml
Add SonarCloud analysis workflow to GitHub Actions..github/workflows/sonarcloud.yml
run-all-tests.yml
Simplify doctest command in GitHub Actions workflow..github/workflows/run-all-tests.yml
sonar-project.properties
Add SonarCloud project configuration file.sonar-project.properties
pyproject.toml
Configure rstcheck tool for documentation validation.pyproject.toml
rstchecktool configuration for documentation checks.Summary by CodeRabbit
Release Notes
New Features
findfunction to locate specific instances within objects, improving utility functions.Documentation
Bug Fixes
Chores
Style