503 typechecking fails with ty and pyrefly - #526
Conversation
- Updated KML parsing to remove unnecessary encoding. - Added type hints to function parameters and return types in various modules. - Improved error handling and assertions in KML document processing. - Refined type casting and removed redundant casts in geometry handling. - Enhanced type checking for XML elements and attributes. - Updated project dependencies and Python version requirements in pyproject.toml. - Improved test cases for geometry and KML handling to ensure type safety.
Raise the minimum supported version to 3.10, drop it from CI matrices, tox.ini, and .sourcery.yaml, and let ruff migrate Optional[X]/Union[X, Y] annotations to the X | None / X | Y syntax across the codebase and tests. Pin hypothesis to <6.156 in the tests extra since newer releases ship no wheel for Python 3.15 yet. Swap the mirrors-mypy pre-commit hook for local ty/pyrefly hooks, matching the type checkers already used in CI and completing the migration away from mypy. Reword two comments that false-positived on pygrep's type-annotations-not-comments check. Also split coordinates_subelement and create_multigeometry's complex blocks into private helpers to bring their cyclomatic complexity back under the complexipy threshold. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cleder, your pull request is larger than the review limit of 150000 diff characters
This comment has been minimized.
This comment has been minimized.
for more information, see https://pre-commit.ci
|
Tick the box to add this pull request to the merge queue (same as
|
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR raises the minimum supported Python version to 3.10, replaces mypy with ChangesPython 3.10 / typing modernization
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary
|
|
Failed to generate code suggestions for PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request drops support for Python 3.9, updates the codebase to Python 3.10+, and migrates the static type checking toolchain from mypy to ty and pyrefly. It also modernizes type annotations across the project by replacing Union and Optional with the pipe (|) syntax. The review feedback identifies a potential runtime TypeError in fastkml/gx/track.py when handling the angles iterable, and points out a configuration error in pyproject.toml where the kebab-case key ignore-missing-imports is used instead of the snake_case ignore_missing_imports required by pyrefly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
fastkml/gx/track.py (192-203)
The parameter angles is typed as Iterable[PointType] | None = None. If angles is not provided (defaulting to None) or is passed as a non-sequence iterable (such as a generator), calling len(angles) and indexing angles[i] will raise a TypeError at runtime.
To prevent runtime crashes, convert angles to a list if it is not None, and safely check the index against its length.
if not track_items and whens and coords:
# `whens`/`coords` form mandatory pairs (a track item always
# needs a timestamp and a coordinate); `angles` may be shorter
# and defaults to Angle()'s all-zero heading/tilt/roll.
angles_list = list(angles) if angles is not None else []
track_items = [
TrackItem(
when=when,
coord=geo.Point(*coord),
angle=Angle(*angles_list[i]) if i < len(angles_list) else Angle(),
)
for i, (when, coord) in enumerate(zip(whens, coords, strict=False))
]
pyproject.toml (241)
According to the migration guide added in this PR (docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md), pyrefly's TOML keys are snake_case even though its CLI flags are kebab-case.
Therefore, the key ignore-missing-imports should be written as ignore_missing_imports to ensure it is correctly parsed and not silently ignored by pyrefly.
ignore_missing_imports = ["*"]
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| # Create the KML object to store the parsed result | ||
| # Read in the KML string | ||
| k = kml.KML.from_string(doc.encode("utf-8")) | ||
| k = kml.KML.from_string(doc) |
There was a problem hiding this comment.
Lack of error handling for KML parsing
The code directly parses the KML string using kml.KML.from_string(doc) without any error handling. If the input string is malformed or contains invalid XML, this may raise an exception and terminate execution unexpectedly.
Recommended solution:
Wrap the parsing operation in a try-except block to handle potential parsing errors gracefully:
try:
k = kml.KML.from_string(doc)
except Exception as e:
print(f"Failed to parse KML: {e}")
# Optionally handle or re-raise|
|
||
| cs_kml = KML.parse(examples_dir / "gx_cascading_style.kml", validate=False) | ||
| document = find(cs_kml, of_type=Document) | ||
| assert document is not None # noqa: S101 | ||
| # gx_cascading_style is a dynamic attribute added to Document by the | ||
| # registry.register() call above; Document's type doesn't declare it. | ||
| for cascading_style in document.gx_cascading_style: | ||
| kml_style = cascading_style.style | ||
| kml_style.id = cascading_style.id |
There was a problem hiding this comment.
The code assumes that document.gx_cascading_style exists and is iterable, and that each cascading_style has a style attribute. If the KML file is malformed or the registry registration fails, this could raise an AttributeError or TypeError at runtime.
Recommended solution:
Add explicit error handling to check for the existence and type of document.gx_cascading_style and the presence of the style attribute before iterating:
if hasattr(document, 'gx_cascading_style') and isinstance(document.gx_cascading_style, list):
for cascading_style in document.gx_cascading_style:
if hasattr(cascading_style, 'style') and cascading_style.style is not None:
kml_style = cascading_style.style
kml_style.id = getattr(cascading_style, 'id', None)
document.styles.append(kml_style)
else:
raise RuntimeError('gx_cascading_style attribute missing or invalid on Document')| **kwargs: Any, | ||
| ) -> None: | ||
| """ |
There was a problem hiding this comment.
The length parameter is assigned directly to self.length without type validation or conversion. If a non-integer value is passed, this could lead to runtime errors or inconsistent behavior elsewhere in the codebase.
Recommendation:
Add explicit type checking or conversion for length:
if length is not None:
try:
self.length = int(length)
except (TypeError, ValueError):
raise ValueError(f"length must be an integer, got {length!r}")
else:
self.length = NoneThis ensures that self.length is always an integer or None, improving robustness.
| ns=ns, | ||
| name_spaces=name_spaces, | ||
| strict=strict, | ||
| element=cast( | ||
| "Element", | ||
| config.etree.fromstring(string), | ||
| ), | ||
| element=config.etree.fromstring(string), | ||
| ) |
There was a problem hiding this comment.
The call to config.etree.fromstring(string) does not include any error handling. If the input string is not valid XML, this will raise an exception (such as XMLSyntaxError), which will propagate up and may cause the application to crash or behave unpredictably.
Recommendation:
Wrap the call in a try-except block to catch XML parsing errors and raise a more descriptive exception or handle the error gracefully. For example:
try:
element = config.etree.fromstring(string)
except Exception as exc:
raise ValueError(f"Failed to parse XML string: {exc}") from exc| def populate_element( | ||
| self, | ||
| element: Element, | ||
| precision: Optional[int] = None, | ||
| precision: int | None = None, | ||
| verbosity: Verbosity = Verbosity.normal, | ||
| ) -> None: | ||
| """ |
There was a problem hiding this comment.
The populate_element method iterates over registry items and calls item.set_element for each. If any registry item is misconfigured or if set_element raises an exception, there is no error handling or logging, which could make debugging difficult.
Recommendation:
Consider adding error handling or logging around the call to item.set_element to improve maintainability and debuggability, especially if the registry is extended or used incorrectly. For example:
for item in registry.get(self.__class__):
try:
item.set_element(...)
except Exception as exc:
logger.error(f"Error setting element for {item.attr_name}: {exc}")
raise| def time_stamp(self) -> KmlDateTime | None: | ||
| """Return the timestamp.""" | ||
| return self.times.timestamp if isinstance(self.times, TimeStamp) else None | ||
|
|
||
| @property | ||
| def begin(self) -> Optional[KmlDateTime]: | ||
| def begin(self) -> KmlDateTime | None: | ||
| """Return the start time of a time span.""" | ||
| return self.times.begin if isinstance(self.times, TimeSpan) else None | ||
|
|
||
| @property | ||
| def end(self) -> Optional[KmlDateTime]: | ||
| def end(self) -> KmlDateTime | None: | ||
| """Return the end time of a time span.""" | ||
| return self.times.end if isinstance(self.times, TimeSpan) else None |
There was a problem hiding this comment.
The property methods (time_stamp, begin, end) rely on self.times being either TimeSpan, TimeStamp, or None. If self.times is set to an unexpected type, these properties will silently return None, potentially masking bugs and making debugging difficult.
Recommendation: Consider adding explicit type validation for self.times in the class initializer or setter, or raise an exception in the property methods if self.times is not None and not an expected type. This will make errors more visible and improve robustness.
| ) | ||
|
|
||
| @property | ||
| def geometry(self) -> Optional[Point]: | ||
| def geometry(self) -> Point | None: | ||
| """Return a Point representation of the geometry.""" | ||
| if not self: | ||
| return None |
There was a problem hiding this comment.
Non-robust error handling in Location.geometry property
The use of assertions (assert self.longitude is not None, assert self.latitude is not None) in the geometry property is not robust. If these values are None, an AssertionError will be raised, which is not user-friendly and may not be caught in production environments. Consider raising a ValueError with a clear message or handling the case gracefully:
if self.longitude is None or self.latitude is None:
raise ValueError("Longitude and latitude must be set to compute geometry.")This approach improves error reporting and maintainability.
|
|
||
| _default_nsid = config.KML | ||
|
|
||
| target_href: Optional[str] | ||
| source_href: Optional[str] | ||
| target_href: str | None | ||
| source_href: str | None | ||
|
|
||
| def __init__( | ||
| self, | ||
| ns: Optional[str] = None, | ||
| name_spaces: Optional[dict[str, str]] = None, | ||
| id: Optional[str] = None, | ||
| target_id: Optional[str] = None, | ||
| target_href: Optional[str] = None, | ||
| source_href: Optional[str] = None, | ||
| ns: str | None = None, | ||
| name_spaces: dict[str, str] | None = None, | ||
| id: str | None = None, | ||
| target_id: str | None = None, | ||
| target_href: str | None = None, | ||
| source_href: str | None = None, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| """Create a new Alias.""" |
There was a problem hiding this comment.
Potential issue with empty string values in Alias constructor
The constructor uses clean_string for target_href and source_href, but does not check if the cleaned values are empty strings. This could result in storing empty strings instead of None, which may affect downstream logic that expects None for unset values. Consider normalizing empty strings to None after cleaning:
self.target_href = clean_string(target_href) or None
self.source_href = clean_string(source_href) or NoneThis ensures unset values are consistently represented as None.
|
|
||
| def __init__( | ||
| self, | ||
| ns: Optional[str] = None, | ||
| name_spaces: Optional[dict[str, str]] = None, | ||
| objects: Optional[Iterable[_XMLObject]] = None, | ||
| ns: str | None = None, | ||
| name_spaces: dict[str, str] | None = None, | ||
| objects: Iterable[_XMLObject] | None = None, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| """ |
There was a problem hiding this comment.
Potential Type Safety Issue in _UpdateAction __init__
The objects parameter is accepted as an Iterable[_XMLObject] | None, but there is no validation to ensure that all items in the iterable are instances of _XMLObject. Passing an incorrect type could lead to runtime errors later when the objects are used.
Recommendation:
Add explicit type validation for the objects parameter:
if objects:
for obj in objects:
if not isinstance(obj, _XMLObject):
raise TypeError(f"All objects must be instances of _XMLObject, got {type(obj)}")
self.objects = list(objects)
else:
self.objects = []This will ensure only valid objects are accepted and errors are caught early.
|
|
||
| _default_nsid = config.KML | ||
|
|
||
| target_href: Optional[str] | ||
| operations: list[Union[Create, Delete, Change]] | ||
| target_href: str | None | ||
| operations: list[Create | Delete | Change] | ||
|
|
||
| def __init__( | ||
| self, | ||
| ns: Optional[str] = None, | ||
| name_spaces: Optional[dict[str, str]] = None, | ||
| target_href: Optional[str] = None, | ||
| operations: Optional[Iterable[Union[Create, Delete, Change]]] = None, | ||
| ns: str | None = None, | ||
| name_spaces: dict[str, str] | None = None, | ||
| target_href: str | None = None, | ||
| operations: Iterable[Create | Delete | Change] | None = None, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| """ |
There was a problem hiding this comment.
Lack of Type Validation for operations in Update __init__
The operations parameter is expected to be an iterable of Create, Delete, or Change instances, but there is no validation to enforce this. If an incorrect type is passed, it may cause runtime errors when the operations are processed.
Recommendation:
Add type validation for the operations parameter:
if operations:
for op in operations:
if not isinstance(op, (Create, Delete, Change)):
raise TypeError(f"All operations must be Create, Delete, or Change instances, got {type(op)}")
self.operations = list(operations)
else:
self.operations = []This will ensure only valid operations are accepted.
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 3 minor |
| ErrorProne | 30 high |
| Security | 1 high |
| Complexity | 1 medium |
🟢 Metrics 9 complexity · -12 duplication
Metric Results Complexity 9 Duplication -12
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
PR Summary by QodoFix ty/pyrefly typechecking; drop Python 3.9; harden etree typing
AI Description
Diagram
High-Level Assessment
Files changed (63)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates FastKML’s supported/tooling baseline to resolve typechecking failures by migrating from mypy to Astral’s ty and Meta’s pyrefly, while also dropping Python 3.9 support. It modernizes typing across the library/tests to Python 3.10+ syntax, updates CI/pre-commit integration, and adds migration documentation for future maintenance.
Changes:
- Drop Python 3.9 support and move the minimum supported version to Python 3.10 across tooling/config/docs.
- Replace mypy configuration and CI checks with
ty+pyrefly, plus targeted ignore/comment updates for the new checkers. - Refactor/modernize many type annotations (PEP 604 unions, overloads, casts) to satisfy the new typecheckers.
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Bumps Flake8 min Python version to 3.10. |
| pyproject.toml | Drops mypy config; adds ty/pyrefly config and typing deps; raises requires-python to 3.10. |
| .github/workflows/run-all-tests.yml | Removes 3.9 from test matrices; replaces mypy step with ty + pyrefly. |
| .pre-commit-config.yaml | Replaces mypy hook with local ty + pyrefly hooks. |
| .sourcery.yaml | Updates Sourcery Python version to 3.10. |
| README.rst | Updates stated Python support and refreshes installation instructions (uv/pip/conda). |
| docs/HISTORY.rst | Notes dropping Python 3.9 support in unreleased changelog. |
| docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md | Adds an internal guide documenting the mypy → ty/pyrefly migration approach and pitfalls. |
| fastkml/config.py | Uses TYPE_CHECKING to type against lxml stubs while keeping runtime fallback to stdlib etree. |
| fastkml/types.py | Uses TYPE_CHECKING to alias Element to lxml.etree._Element for static analysis; keeps runtime Protocol fallback. |
| fastkml/base.py | Updates signatures/return typing and reduces unnecessary casts for etree operations. |
| fastkml/kml.py | Fixes validation typing (validate root element), improves tag typing via cast, and modernizes annotations. |
| fastkml/kml_base.py | Tightens base object typing around id/target_id and modernizes init annotations. |
| fastkml/registry.py | Modernizes Optional typing to PEP 604 unions and updates registry container typing. |
| fastkml/helpers.py | Modernizes typing and adds checker-specific ignore comments where needed. |
| fastkml/utils.py | Adds overloads to find/find_all for better type narrowing and modernizes typing. |
| fastkml/validator.py | Adjusts typing around lxml error logs and ensures validate() works with root elements. |
| fastkml/times.py | Modernizes typing and tightens KmlDateTime.parse return type. |
| fastkml/mixins.py | Modernizes time-related union typing for mixins. |
| fastkml/views.py | Modernizes view object annotations and constructor parameter types. |
| fastkml/styles.py | Modernizes typing across styles and style selectors, including union aliases. |
| fastkml/links.py | Modernizes link/icon typing to PEP 604 unions. |
| fastkml/containers.py | Modernizes container typing and removes unnecessary type ignores. |
| fastkml/features.py | Modernizes feature typing, including geometry unions and constructor annotations. |
| fastkml/data.py | Modernizes schema/data typing and updates union usage. |
| fastkml/model.py | Modernizes model-related typing and property return types. |
| fastkml/overlays.py | Modernizes overlay typing and constructor signatures. |
| fastkml/network_link_control.py | Modernizes typing for update/network link control structures. |
| fastkml/geometry.py | Refactors coordinate formatting, modernizes geometry typing, and tightens multigeometry creation typing. |
| fastkml/gx/track.py | Modernizes typing and refactors track-item construction logic. |
| fastkml/gx/data.py | Modernizes typing for gx simple array field/data. |
| fastkml/atom.py | Modernizes Atom element typing. |
| fastkml/abstract_geometry.py | Modernizes base geometry typing (`AltitudeMode |
| examples/transform_cascading_style.py | Modernizes typing and adds a runtime assertion for narrowing in the example. |
| examples/simple_example.py | Adds type annotations to make the example typechecker-friendly. |
| examples/read_kml.py | Fixes example to pass str (not bytes) to from_string. |
| tests/base.py | Updates optional lxml import style for typing/pyrefly compatibility. |
| tests/config_test.py | Updates optional lxml import style and avoids mutating a shared dict literal. |
| tests/registry_test.py | Updates Optional→union typing and provides a concrete get_kwarg() body for typechecking. |
| tests/times_test.py | Adds ty ignore to a deliberately-invalid construction. |
| tests/overlays_test.py | Fixes a test to use AltitudeMode enum instead of a raw string. |
| tests/helper_test.py | Switches Callable import to collections.abc. |
| tests/data_test.py | Adds ty ignore to **dict splat cases that typecheckers can’t precisely validate. |
| tests/gx/data_test.py | Adds ty ignore to **dict splat cases that typecheckers can’t precisely validate. |
| tests/geometries/point_test.py | Adds ty ignore for deliberately-invalid geometry construction. |
| tests/geometries/geometry_test.py | Replaces len(x) # type: ignore with explicit narrowing assertions; adds ty ignore in invalid-input test. |
| tests/geometries/functions_test.py | Switches Callable import to collections.abc. |
| tests/geometries/boundaries_test.py | Modernizes union typing in helper signature. |
| tests/hypothesis/atom_test.py | Modernizes Optional→union typing for Hypothesis tests. |
| tests/hypothesis/data_test.py | Modernizes Optional/Union typing for Hypothesis tests. |
| tests/hypothesis/feature_test.py | Modernizes Optional/Union typing for Hypothesis tests. |
| tests/hypothesis/geometry_test.py | Modernizes typing and union aliases for Hypothesis geometry tests. |
| tests/hypothesis/kml_test.py | Adds ty ignore for a known list-item typing mismatch. |
| tests/hypothesis/links_test.py | Modernizes typing for Hypothesis link/icon tests. |
| tests/hypothesis/model_test.py | Modernizes typing for Hypothesis model tests. |
| tests/hypothesis/multi_geometry_test.py | Uses zip(..., strict=False) for typechecker compatibility under 3.10+. |
| tests/hypothesis/network_link_control_test.py | Modernizes typing for Hypothesis network link control tests. |
| tests/hypothesis/overlay_test.py | Modernizes typing for Hypothesis overlay tests. |
| tests/hypothesis/style_test.py | Modernizes typing and unions for Hypothesis style tests. |
| tests/hypothesis/times_test.py | Modernizes typing for Hypothesis time tests. |
| tests/hypothesis/views_test.py | Modernizes typing for Hypothesis view tests. |
| tests/hypothesis/gx/data_test.py | Modernizes typing for Hypothesis gx data tests. |
| tests/hypothesis/gx/track_test.py | Modernizes typing for Hypothesis gx track tests. |
| matches = cast("list[Element]", element.xpath(error_entry.path)) | ||
| parent = matches[0].getparent() |
Greptile SummaryThis PR drops Python 3.9 support, replaces mypy with
Confidence Score: 5/5Safe to merge — changes are primarily mechanical type-annotation modernisation with two small correctness fixes in validator.py that were pre-existing latent bugs. The bulk of the diff is semantically equivalent annotation rewrites. The two functional changes in validator.py (.getroot() and adding IndexError) are genuine improvements with no regression risk. The dual TYPE_CHECKING import pattern for Element and etree is a well-understood idiom. No public API shapes change. No files require special attention. fastkml/validator.py has the only non-trivial logic changes and they are straightforward improvements. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TYPE_CHECKING?] -->|Yes| B["Element = lxml.etree._Element\netree = lxml.etree"]
A -->|No – runtime| C{lxml available?}
C -->|Yes| D["etree = lxml.etree\nElement = Protocol"]
C -->|No| E["etree = xml.etree.ElementTree\nElement = Protocol"]
F["validate(file_to_validate=...)"] --> G["etree.parse(file).getroot()"]
G --> H["XMLSchema.assert_(element: _Element)"]
H -->|passes| I["return True"]
H -->|fails| J["handle_validation_error()"]
J --> K["cast Iterable[_LogEntry]"]
K --> L["element.xpath(path)"]
L -->|IndexError or XPathEvalError| M["parent = element"]
L -->|ok| N["parent = matches[0].getparent()"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[TYPE_CHECKING?] -->|Yes| B["Element = lxml.etree._Element\netree = lxml.etree"]
A -->|No – runtime| C{lxml available?}
C -->|Yes| D["etree = lxml.etree\nElement = Protocol"]
C -->|No| E["etree = xml.etree.ElementTree\nElement = Protocol"]
F["validate(file_to_validate=...)"] --> G["etree.parse(file).getroot()"]
G --> H["XMLSchema.assert_(element: _Element)"]
H -->|passes| I["return True"]
H -->|fails| J["handle_validation_error()"]
J --> K["cast Iterable[_LogEntry]"]
K --> L["element.xpath(path)"]
L -->|IndexError or XPathEvalError| M["parent = element"]
L -->|ok| N["parent = matches[0].getparent()"]
Reviews (3): Last reviewed commit: "Fix from_string crashing on XML strings ..." | Re-trigger Greptile |
…om:cleder/fastkml into 503-typechecking-fails-with-ty-and-pyrefly
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR modernizes type annotations by migrating from mypy to ty and pyrefly, but introduces a critical type inconsistency in _BaseObject that will cause false negatives in type checking and potential runtime AttributeErrors on attributes that can be None.
🌟 Strengths
- Comprehensive type annotation modernization across the codebase (switching from
OptionaltoX | None). - Added a thorough migration guide (
docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md).
⚡ Key Risks & Improvements (P1)
- fastkml/kml_base.py: Class-level
idandtarget_idare declared asstr(neverNone), contradicting the__init__which allowsNone, leading to false negative type-checking results and unsafe access assuming non-None.
📈 Risk Diagram
This diagram illustrates the risk of a type annotation mismatch in _BaseObject leading to unsafe attribute access.
sequenceDiagram
participant BO as _BaseObject
participant TC as Type Checker
participant DC as Downstream Code
BO->>BO: Declares id: str (class-level)
BO->>BO: __init__ accepts id: str|None=None
Note over BO: R1(P1): Annotation mismatch<br/>id can be None at runtime
TC->>BO: Reads annotation: id: str
TC->>DC: Infers id is never None
DC->>DC: Accesses obj.id.upper() without None check
Note over DC: Potential AttributeError if id is None
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| _default_nsid = config.KML | ||
|
|
||
| id = None | ||
| target_id = None | ||
| id: str | ||
| target_id: str |
There was a problem hiding this comment.
P1 | Confidence: High
The class-level type annotation declares id and target_id as str (never None), but the __init__ method (unchanged in this PR, signature shown below) still accepts id: str | None = None and target_id: str | None = None.
This inconsistency means type checkers (both ty and pyrefly, which the PR is migrating to) will infer that _BaseObject.id is always a str, allowing unsafe access without None checks. At runtime, these attributes can still be None (when the corresponding constructor argument is omitted or explicitly set to None), leading to AttributeError on methods like getattr or direct attribute access that assume a non‑None string.
All classes inheriting from _BaseObject (e.g., _Geometry, Placemark, Document, Link) share this incorrect annotation, so the ripple effect is broad. The fix is straightforward: change class‑level annotations to str | None.
| _default_nsid = config.KML | |
| id = None | |
| target_id = None | |
| id: str | |
| target_id: str | |
| class _BaseObject(_XMLObject): | |
| ... | |
| id: str | None = None | |
| target_id: str | None = None |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #526 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 80 80
Lines 6579 6544 -35
Branches 164 166 +2
===========================================
- Hits 6579 6543 -36
- Misses 0 1 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fastkml/gx/track.py (1)
188-203: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce matching
whens/coordslengthszip(..., strict=False)will silently drop any extra timestamps or coordinates here, so a caller can lose track points without an error;strict=Truematches the stated pairing invariant.🔧 Suggested fix
TrackItem( when=when, coord=geo.Point(*coord), angle=Angle(*angles[i]) if i < len(angles) else Angle(), ) - for i, (when, coord) in enumerate(zip(whens, coords, strict=False)) + for i, (when, coord) in enumerate(zip(whens, coords, strict=True)) ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fastkml/gx/track.py` around lines 188 - 203, The TrackItem construction in track.py currently uses zip(..., strict=False), which can silently drop extra values and violate the required one-to-one pairing between whens and coords. Update the TrackItem creation logic in the track-building branch to enforce matching lengths by using strict pairing behavior in that zip call, so any mismatch raises instead of truncating data. Keep the existing invariant checks around track_items, whens, and coords consistent with the TrackItem/Angle construction path.
🧹 Nitpick comments (2)
.pre-commit-config.yaml (1)
65-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning
tyvia its official pre-commit hook.
language: systemhooks rely onty/pyreflyalready being installed in the developer's environment (via thetypingextra), so the exact tool version isn't pinned by pre-commit itself — it can silently drift from what CI uses. Astral publishes an officialastral-sh/ty-pre-commitrepo hook with a pinnedrev, which would give reproducible versioning across contributors' machines without requiring a system install.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.pre-commit-config.yaml around lines 65 - 78, The local pre-commit hook setup for ty is using language: system, so its version depends on whatever is installed on each developer machine. Update the hook definition to use ty’s official pre-commit repository with a pinned rev instead of the local system-based entry, and keep the existing pyrefly hook only if it still needs to remain system-managed. Refer to the ty hook block in the pre-commit configuration when making the change.examples/simple_example.py (1)
7-7: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGive the helper a protocol for
featuresandname.objectstill won’t type-check with the recursiveelement.features/feature.nameaccess, and thegetattr(..., "features", None)guard doesn’t narrow it for the type checker. A tiny localProtocolhere would fit better thanobjectorAny.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/simple_example.py` at line 7, The helper signature in print_child_features still uses object, which doesn’t type-check for the recursive element.features and feature.name access. Replace the broad object annotation with a small local Protocol that declares features and name so the type checker can understand the recursive traversal, and keep the guard logic aligned with that protocol instead of relying on getattr narrowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md`:
- Around line 110-124: The pyrefly casing guidance is too broad and should be
narrowed to the specific option naming patterns used in the docs. Update the
wording in the migration guide near the pyrefly TOML examples to explicitly
distinguish the hyphenated keys that remain hyphenated in `pyproject.toml` (for
example `ignore-missing-imports` and `sub-config`) from the underscore-based
keys like `replace_imports_with_any`. Also remove or revise the claim that the
later stub-package example is mismatched, since it already follows the same key
form as the project config.
In `@fastkml/types.py`:
- Around line 58-59: Update the Element protocol’s get method in types.py so it
matches the underlying XML APIs by returning str | None instead of only str.
Adjust the get signature on Element to allow a missing attribute result, and
keep the method name and protocol definition consistent with
lxml.etree._Element.get and xml.etree.ElementTree.Element.get.
In `@pyproject.toml`:
- Around line 139-153: The Pyrefly config is using an underscore key that won’t
be recognized, so the import-to-Any rule is not applied. Update the
[tool.pyrefly] section to use the hyphenated setting name for
replace-imports-with-any, keeping the existing lxml.* value, and leave the rest
of the pyrefly options unchanged.
In `@README.rst`:
- Around line 142-149: The pip install example omits the optional lxml
dependency and is inconsistent with the uv example. Update the installation
snippet in README.rst so the pip command matches the package extra used in the
uv example, and keep the reference consistent with the fastkml installation
guidance.
---
Outside diff comments:
In `@fastkml/gx/track.py`:
- Around line 188-203: The TrackItem construction in track.py currently uses
zip(..., strict=False), which can silently drop extra values and violate the
required one-to-one pairing between whens and coords. Update the TrackItem
creation logic in the track-building branch to enforce matching lengths by using
strict pairing behavior in that zip call, so any mismatch raises instead of
truncating data. Keep the existing invariant checks around track_items, whens,
and coords consistent with the TrackItem/Angle construction path.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Around line 65-78: The local pre-commit hook setup for ty is using language:
system, so its version depends on whatever is installed on each developer
machine. Update the hook definition to use ty’s official pre-commit repository
with a pinned rev instead of the local system-based entry, and keep the existing
pyrefly hook only if it still needs to remain system-managed. Refer to the ty
hook block in the pre-commit configuration when making the change.
In `@examples/simple_example.py`:
- Line 7: The helper signature in print_child_features still uses object, which
doesn’t type-check for the recursive element.features and feature.name access.
Replace the broad object annotation with a small local Protocol that declares
features and name so the type checker can understand the recursive traversal,
and keep the guard logic aligned with that protocol instead of relying on
getattr narrowing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: afe1f0ab-34ac-4c21-9969-c5c12b78369b
📒 Files selected for processing (63)
.github/workflows/run-all-tests.yml.pre-commit-config.yaml.sourcery.yamlREADME.rstdocs/HISTORY.rstdocs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.mdexamples/read_kml.pyexamples/simple_example.pyexamples/transform_cascading_style.pyfastkml/abstract_geometry.pyfastkml/atom.pyfastkml/base.pyfastkml/config.pyfastkml/containers.pyfastkml/data.pyfastkml/features.pyfastkml/geometry.pyfastkml/gx/data.pyfastkml/gx/track.pyfastkml/helpers.pyfastkml/kml.pyfastkml/kml_base.pyfastkml/links.pyfastkml/mixins.pyfastkml/model.pyfastkml/network_link_control.pyfastkml/overlays.pyfastkml/registry.pyfastkml/styles.pyfastkml/times.pyfastkml/types.pyfastkml/utils.pyfastkml/validator.pyfastkml/views.pypyproject.tomltests/base.pytests/config_test.pytests/data_test.pytests/geometries/boundaries_test.pytests/geometries/functions_test.pytests/geometries/geometry_test.pytests/geometries/point_test.pytests/gx/data_test.pytests/helper_test.pytests/hypothesis/atom_test.pytests/hypothesis/data_test.pytests/hypothesis/feature_test.pytests/hypothesis/geometry_test.pytests/hypothesis/gx/data_test.pytests/hypothesis/gx/track_test.pytests/hypothesis/kml_test.pytests/hypothesis/links_test.pytests/hypothesis/model_test.pytests/hypothesis/multi_geometry_test.pytests/hypothesis/network_link_control_test.pytests/hypothesis/overlay_test.pytests/hypothesis/style_test.pytests/hypothesis/times_test.pytests/hypothesis/views_test.pytests/overlays_test.pytests/registry_test.pytests/times_test.pytox.ini
| ### 2. pyrefly's TOML keys are snake_case even though its CLI flags are kebab-case | ||
|
|
||
| This is the single most time-consuming mistake to make. `pyrefly check --replace-imports-with-any 'lxml.*'` works from the CLI. Writing the "obvious" TOML equivalent: | ||
|
|
||
| ```toml | ||
| [tool.pyrefly] | ||
| replace-imports-with-any = ["lxml.*"] # WRONG — silently different key | ||
| ``` | ||
|
|
||
| ...does not raise an error from `pyrefly check` in some code paths, but it **does** hard-fail with `pyrefly dump-config` (`unknown variant 'replace-imports-with-any'... Fatal configuration error`), and depending on invocation order this can also break `pyrefly check` itself later. The correct TOML key uses underscores: | ||
|
|
||
| ```toml | ||
| [tool.pyrefly] | ||
| replace_imports_with_any = ["lxml.*"] | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files of interest ==\n'
git ls-files 'docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md' 'pyproject.toml' '**/pyproject.toml' | sed 's#^`#-` #'
printf '\n== Guide excerpts ==\n'
nl -ba docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md | sed -n '100,190p'
printf '\n== pyproject.toml mentions of pyrefly ==\n'
rg -n --no-heading 'pyrefly|ignore-missing-imports|replace-imports-with-any|replace_imports_with_any|stub' pyproject.toml . -g 'pyproject.toml' -g '!**/node_modules/**' || trueRepository: cleder/fastkml
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Guide excerpts ==\n'
nl -ba docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md | sed -n '100,190p'
printf '\n== Root pyproject.toml ==\n'
if [ -f pyproject.toml ]; then
nl -ba pyproject.toml | sed -n '1,260p'
fi
printf '\n== Repo-wide pyrefly key search ==\n'
rg -n --no-heading 'ignore-missing-imports|replace-imports-with-any|replace_imports_with_any|tool\.pyrefly|pyrefly' . -g 'pyproject.toml' -g '*.toml' -g '*.md' || trueRepository: cleder/fastkml
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Guide excerpt ==\n'
sed -n '100,190p' docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md
printf '\n== Root pyproject.toml excerpt ==\n'
sed -n '1,260p' pyproject.toml
printf '\n== Repo-wide pyrefly-related mentions ==\n'
rg -n --no-heading 'ignore-missing-imports|replace-imports-with-any|replace_imports_with_any|tool\.pyrefly|pyrefly' . -g 'pyproject.toml' -g '*.toml' -g '*.md' || trueRepository: cleder/fastkml
Length of output: 25002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md'), Path('pyproject.toml')]:
print(f"\n== {path} ==")
text = path.read_text()
if path.name == 'mypy-to-ty-and-pyrefly-migration.md':
lines = text.splitlines()
for i in range(100, min(190, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
else:
for i, line in enumerate(text.splitlines()[:260], 1):
print(f"{i:4d}: {line}")
PY
printf '\n== pyrefly keys in TOML files ==\n'
python3 - <<'PY'
from pathlib import Path
import re
patterns = [
re.compile(r'ignore-missing-imports'),
re.compile(r'replace-imports-with-any'),
re.compile(r'replace_imports_with_any'),
re.compile(r'^\s*\[tool\.pyrefly\]'),
re.compile(r'pyrefly'),
]
for path in Path('.').rglob('*.toml'):
text = path.read_text(errors='ignore')
if any(p.search(text) for p in patterns):
print(f'-- {path}')
for i, line in enumerate(text.splitlines(), 1):
if any(p.search(line) for p in patterns):
print(f'{i}: {line}')
PYRepository: cleder/fastkml
Length of output: 15218
Clarify the pyrefly key-casing rule. pyproject.toml mixes hyphenated options like ignore-missing-imports and sub-config with underscored options like replace_imports_with_any, so “TOML keys are snake_case” is too broad. Narrow this wording to the specific option names, and drop the claim that the later stub-package example is mismatched; it already uses the same key form as the project config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/codedocs/guides/mypy-to-ty-and-pyrefly-migration.md` around lines 110 -
124, The pyrefly casing guidance is too broad and should be narrowed to the
specific option naming patterns used in the docs. Update the wording in the
migration guide near the pyrefly TOML examples to explicitly distinguish the
hyphenated keys that remain hyphenated in `pyproject.toml` (for example
`ignore-missing-imports` and `sub-config`) from the underscore-based keys like
`replace_imports_with_any`. Also remove or revise the claim that the later
stub-package example is mismatched, since it already follows the same key form
as the project config.
| def get(self, tag: str) -> str: | ||
| """Get the value of the tag.""" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
lxml etree _Element get method return type Optional str
💡 Result:
The lxml.etree._Element.get(key, default=None) method returns the value of an attribute if it exists, or the provided default value if it does not [1][2][3]. In practice, the return type is typically a string (str) if the attribute is found [4], or the value provided in the default argument if the attribute is absent [1][2]. Since the default parameter defaults to None, the method effectively returns an Optional[str] (a string or None) [1][2][3]. This behavior aligns with the standard Python ElementTree API, which lxml implements [5]. When an attribute does not exist and no explicit default is provided, the method returns None [1][2].
Citations:
- 1: https://lxml.de/4.3/api/lxml.etree._Element-class.html
- 2: https://lxml.de/4.4/api/lxml.etree._Element-class.html
- 3: https://lxml.de/5.4/apidoc/lxml.etree.html
- 4: https://webscraping.ai/faq/lxml/how-do-i-extract-attribute-values-from-html-elements-with-lxml
- 5: https://lxml.de/3.6/api/xml.etree.ElementTree.Element-class.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file with line numbers
sed -n '1,140p' fastkml/types.py | cat -n
echo
echo "--- Usage search for Element.get in the codebase ---"
rg -n "\.get\(" fastkml | head -n 80
echo
echo "--- Search for Element protocol/type alias references ---"
rg -n "TYPE_CHECKING|Protocol|Element =" fastkml/types.py fastkml -g '*.py'Repository: cleder/fastkml
Length of output: 6120
Change Element.get() to return str | None
lxml.etree._Element.get() and xml.etree.ElementTree.Element.get() both return None when the attribute is missing, so this protocol should match that behavior.
🔧 Suggested fix
- def get(self, tag: str) -> str:
+ def get(self, tag: str) -> str | None:
"""Get the value of the tag."""📝 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.
| def get(self, tag: str) -> str: | |
| """Get the value of the tag.""" | |
| def get(self, tag: str) -> str | None: | |
| """Get the value of the tag.""" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fastkml/types.py` around lines 58 - 59, Update the Element protocol’s get
method in types.py so it matches the underlying XML APIs by returning str | None
instead of only str. Adjust the get signature on Element to allow a missing
attribute result, and keep the method name and protocol definition consistent
with lxml.etree._Element.get and xml.etree.ElementTree.Element.get.
| [tool.pyrefly] | ||
| ignore-missing-imports = [ | ||
| "*", | ||
| ] | ||
| preset = "strict" | ||
| replace_imports_with_any = [ | ||
| "lxml.*", | ||
| ] | ||
| sub-config = [ | ||
| { errors = { missing-attribute = "ignore" }, matches = "examples/**/*" }, | ||
| { errors = { missing-attribute = "ignore" }, matches = "tests/**/*" }, | ||
| ] | ||
|
|
||
| [[tool.mypy.overrides]] | ||
| disable_error_code = "union-attr" | ||
| module = "tests.*" | ||
| [tool.pyrefly.errors] | ||
| missing-override-decorator = "ignore" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify pyrefly recognizes the configured key and dumps expected settings
pip show pyrefly 2>/dev/null || pip install pyrefly
pyrefly dump-config 2>&1 | grep -i -A2 "replace"Repository: cleder/fastkml
Length of output: 443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pyproject excerpt ==\n'
sed -n '130,160p' pyproject.toml
printf '\n== pyrefly install/version ==\n'
python3 - <<'PY'
import importlib.metadata as m
try:
print(m.version("pyrefly"))
except Exception as e:
print(f"pyrefly version unavailable: {e}")
PY
printf '\n== pyrefly help ==\n'
pyrefly --help 2>&1 | sed -n '1,120p'
printf '\n== pyrefly dump-config (repo root) ==\n'
pyrefly dump-config 2>&1 | sed -n '1,220p'Repository: cleder/fastkml
Length of output: 835
🏁 Script executed:
python3 - <<'PY'
import importlib.util, importlib.metadata as m, sys, os, json
print("pyrefly version:", m.version("pyrefly"))
spec = importlib.util.find_spec("pyrefly")
print("pyrefly module spec:", spec)
if spec and spec.origin:
print("pyrefly origin:", spec.origin)
print("pyrefly package dir:", os.path.dirname(spec.origin))
PY
python3 -m pyrefly --help 2>&1 | sed -n '1,120p'
python3 -m pyrefly dump-config 2>&1 | sed -n '1,240p'Repository: cleder/fastkml
Length of output: 3188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PYREFLY_DIR="$(python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("pyrefly")
print(spec.submodule_search_locations[0] if spec and spec.submodule_search_locations else "")
PY
)"
printf 'pyrefly dir: %s\n' "$PYREFLY_DIR"
printf '\n== candidate key occurrences ==\n'
grep -RIn --include='*.py' --include='*.toml' --include='*.md' \
-E 'replace[_-]imports[_-]with[_-]any|ignore[_-]missing[_-]imports|sub[_-]config|missing[_-]override[_-]decorator' \
"$PYREFLY_DIR" | sed -n '1,240p'
printf '\n== config-related files ==\n'
find "$PYREFLY_DIR" -maxdepth 3 \( -iname '*config*' -o -iname '*schema*' -o -iname '*toml*' -o -iname '*md' \) | sed -n '1,240p'Repository: cleder/fastkml
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PYREFLY_DIR="$(python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("pyrefly")
print(spec.submodule_search_locations[0] if spec and spec.submodule_search_locations else "")
PY
)"
find "$PYREFLY_DIR" -maxdepth 4 | sed -n '1,240p'Repository: cleder/fastkml
Length of output: 684
🌐 Web query:
pyrefly 1.1.1 configuration replace imports with any key spelling hyphen underscore dump-config migration guide
💡 Result:
In Pyrefly 1.1.1, you can configure import handling and inspect your configuration using the following methods: Replace Imports with Any You can configure Pyrefly to replace specific imports with typing.Any, which is useful for improving performance or handling incomplete stubs [1]. This is done using the replace-imports-with-any configuration option, which accepts a list of module wildcard patterns [1]. In your pyrefly.toml or pyproject.toml ([tool.pyrefly] section), add it as follows: # pyrefly.toml replace-imports-with-any = [ "some_module.*", "another.module.name" ] Configuration Syntax Pyrefly configuration keys use hyphens (-) rather than underscores (_) [2][3]. Supported configuration files include pyrefly.toml (placed at the project root) or pyproject.toml (within the [tool.pyrefly] section) [2][3]. Debugging Configuration To inspect the resolved import configuration Pyrefly is applying to your files, you can use the dump-config command [4]. Replace check with dump-config in your standard Pyrefly command invocation to see the effective settings for the files being checked [4]: pyrefly dump-config Migration Guide If you are upgrading your Pyrefly version, the recommended approach to managing new type errors is to use error suppression rather than manual migration scripts [5]. After upgrading to 1.1.1, you can run the following sequence to safely silence errors while you work: 1. pyrefly check --suppress-errors (adds # pyrefly: ignore comments to your code) [5] 2. Run your code formatter [5] 3. pyrefly check --remove-unused-ignores [5] 4. Repeat as necessary until the codebase is clean [5]
Citations:
- 1: https://deepwiki.com/facebook/pyrefly/3.2-import-resolution-configuration
- 2: https://github.com/facebook/pyrefly/blob/b6dd8b08/website/docs/configuration.mdx
- 3: https://deepwiki.com/facebook/pyrefly/3.1-configuration-files
- 4: https://pyrefly.org/en/docs/import-resolution/
- 5: https://github.com/facebook/pyrefly/releases/tag/1.1.1
Use replace-imports-with-any here ``
Pyrefly 1.1.1 expects hyphenated config keys, so replace_imports_with_any won’t be picked up in this block. Use replace-imports-with-any to keep the import-to-Any rule applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 139 - 153, The Pyrefly config is using an
underscore key that won’t be recognized, so the import-to-Any rule is not
applied. Update the [tool.pyrefly] section to use the hyphenated setting name
for replace-imports-with-any, keeping the existing lxml.* value, and leave the
rest of the pyrefly options unchanged.
| uv add "fastkml[lxml]" | ||
|
|
||
| Using pip | ||
| --------- | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| pip install fastkml |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant docs and package metadata.
git ls-files | rg '^(README\.rst|pyproject\.toml|setup\.cfg|setup\.py|requirements.*|.*\.toml)$'
# Show the target README section with line numbers.
sed -n '130,160p' README.rst | cat -n
# Inspect project metadata for extras / optional dependencies.
for f in pyproject.toml setup.cfg setup.py; do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,260p' "$f" | cat -n
fi
done
# Search for lxml / extras references across the repo.
rg -n --hidden --glob '!*.pyc' --glob '!*.png' --glob '!*.jpg' --glob '!*.gif' \
'lxml|extra|optional-depend|fastkml\[lxml\]|pip install fastkml|uv add "fastkml\[lxml\]"' .Repository: cleder/fastkml
Length of output: 21516
Keep the lxml extra in the pip example. pip install fastkml leaves out the optional lxml dependency, so this should match the uv example: pip install "fastkml[lxml]".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.rst` around lines 142 - 149, The pip install example omits the
optional lxml dependency and is inconsistent with the uv example. Update the
installation snippet in README.rst so the pip command matches the package extra
used in the uv example, and keep the reference consistent with the fastkml
installation guidance.
Fix a real correctness bug flagged by three independent bots: Track's whens/coords zip used strict=False, silently truncating data despite the code's own comment stating they're mandatory pairs. Now strict=True. Finish the Optional[X] -> X | None modernization for the handful of TYPE_CHECKING-only forward references ruff safely declined to touch (rewriting them unquoted would evaluate str.__or__ at def time and raise, since these files don't use `from __future__ import annotations`); quote the whole expression instead, matching the project's existing pattern for forward refs. Fix an uncaught IndexError in validator.handle_validation_error: an xpath match list can legitimately come back empty without raising XPathEvalError, so matches[0] could crash instead of falling back to the element itself. Restore the ~15 lines of [tool.pyrefly]/[tool.ty] config comments that pre-commit.ci's pyprojectsort run silently stripped after the last push, and add a `ci: skip: [pyprojectsort]` block so that keeps happening - pyprojectsort has no option to preserve comments, so it stays useful locally but is kept out of the bot's autofix path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lxml's fromstring() rejects a Python str that contains an <?xml ... encoding="..."?> declaration, raising ValueError. This regressed in 2bb5aed, which dropped from_string's .encode("utf-8") call as "unnecessary" - it was load-bearing. examples/read_kml.py (whose doc string has an encoding declaration) surfaced this via CI's doctest-lxml job. Encode to bytes before parsing so lxml can honor the declared encoding itself; verified this also still works with the stdlib xml.etree.ElementTree fallback backend. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| def __init__( | ||
| self, | ||
| *, | ||
| ns: Optional[str] = None, | ||
| name_spaces: Optional[dict[str, str]] = None, | ||
| id: Optional[str] = None, | ||
| target_id: Optional[str] = None, | ||
| altitude_mode: Optional[AltitudeMode] = None, | ||
| tracks: Optional[Iterable[Track]] = None, | ||
| interpolate: Optional[bool] = None, | ||
| ns: str | None = None, | ||
| name_spaces: dict[str, str] | None = None, | ||
| id: str | None = None, | ||
| target_id: str | None = None, | ||
| altitude_mode: AltitudeMode | None = None, | ||
| tracks: Iterable[Track] | None = None, | ||
| interpolate: bool | None = None, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| """ |
There was a problem hiding this comment.
Potential Type Safety Issue in MultiTrack.init
The constructor for MultiTrack does not validate that all elements in the tracks iterable are instances of the Track class. If a non-Track object is passed, it will be stored in self.tracks, potentially causing runtime errors in downstream methods (e.g., when calling track.geometry).
Recommendation:
Add explicit type validation for the tracks argument:
self.tracks = [t for t in tracks if isinstance(t, Track)] if tracks else []This will ensure that only valid Track objects are included, improving robustness and preventing subtle bugs.
| def get_ns(obj: "_XMLObject", value: object) -> str: | ||
| """Get the namespace of an attribute, fall back on the objects namespace.""" | ||
| try: | ||
| return obj.name_spaces.get(value.get_ns_id(), "") # type: ignore[attr-defined] | ||
| return obj.name_spaces.get(value.get_ns_id(), "") # type: ignore[attr-defined] # pyrefly: ignore # ty: ignore[unresolved-attribute] | ||
| except AttributeError: | ||
| return obj.ns | ||
|
|
There was a problem hiding this comment.
Overly Broad Exception Handling in get_ns
The function catches all AttributeError exceptions, which could mask unrelated attribute errors on obj or value. This may make debugging more difficult if an unexpected attribute is missing. Consider narrowing the exception handling to only the expected case, or explicitly checking for the attribute before accessing it:
if hasattr(value, 'get_ns_id'):
return obj.name_spaces.get(value.get_ns_id(), "")
return obj.nsThis approach improves maintainability and debuggability.
| *, | ||
| attr_name: str, | ||
| verbosity: Verbosity, | ||
| default: Optional[Any], | ||
| ) -> Optional[Any]: | ||
| default: Any | None, | ||
| ) -> Any | None: | ||
| """ | ||
| Get the value of an attribute from an object. | ||
|
|
There was a problem hiding this comment.
Ambiguous Return Logic in get_value
The return statement combines two conditions in a single line, which may reduce readability and could lead to subtle bugs if the logic is changed in the future:
return None if value == default and verbosity == Verbosity.terse else valueConsider making the logic more explicit for maintainability:
if value == default and verbosity == Verbosity.terse:
return None
return valueThis makes the function's intent clearer and reduces the risk of future errors.
|
|
||
| @lru_cache(maxsize=16) | ||
| def get_schema_parser( | ||
| schema: Optional[pathlib.Path] = None, | ||
| schema: pathlib.Path | None = None, | ||
| ) -> "etree.XMLSchema": | ||
| """ | ||
| Parse the XML schema. |
There was a problem hiding this comment.
Potential unhandled exception in schema parsing
If the schema file is missing or unreadable, config.etree.parse(schema) will raise an exception that is not caught. This could cause the application to fail unexpectedly. Consider wrapping the parsing logic in a try-except block and logging or handling the error gracefully:
try:
return config.etree.XMLSchema(config.etree.parse(schema))
except (OSError, config.etree.XMLSchemaParseError) as e:
logger.error("Failed to parse schema: %s", e)
raise| if file_to_validate is not None: | ||
| element = config.etree.parse(file_to_validate) | ||
| element = config.etree.parse(file_to_validate).getroot() |
There was a problem hiding this comment.
No error handling for file parsing
If config.etree.parse(file_to_validate) fails due to a malformed or missing file, an exception will be raised and not caught, resulting in abrupt failure. To improve robustness, wrap the parsing in a try-except block and log the error:
try:
element = config.etree.parse(file_to_validate).getroot()
except (OSError, config.etree.XMLSyntaxError) as e:
logger.error("Failed to parse file: %s", e)
raise
There was a problem hiding this comment.
(2 files improve in Code Health)
Our agent can fix these. Install it.
Gates Passed
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| utils.py | 9.69 → 10.00 | Complex Conditional |
| overlay_test.py | 8.82 → 9.39 | Code Duplication |
Quality Gate Profile: Customizable Safeguards
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| # lxml rejects a `str` with an XML encoding declaration | ||
| # (`<?xml ... encoding="..."?>`); bytes let it honor the | ||
| # declared encoding itself. | ||
| element=config.etree.fromstring(string.encode("utf-8")), |
There was a problem hiding this comment.
Encoding mismatch in fastkml/base.py causes config.etree.fromstring() to misdecode XML documents that declare a non-UTF-8 encoding, such as <?xml version="1.0" encoding="ISO-8859-1"?>, and silently corrupt non-ASCII characters into mojibake. Strip or rewrite the XML encoding declaration to UTF-8 before encoding, or pass the string directly when no encoding declaration is present and only encode when necessary.
Prompt for LLM
File fastkml/base.py:
Line 482:
Encoding mismatch in fastkml/base.py causes `config.etree.fromstring()` to misdecode XML documents that declare a non-UTF-8 encoding, such as `<?xml version="1.0" encoding="ISO-8859-1"?>`, and silently corrupt non-ASCII characters into mojibake. Strip or rewrite the XML encoding declaration to `UTF-8` before encoding, or pass the string directly when no encoding declaration is present and only encode when necessary.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Summary by CodeRabbit
New Features
Bug Fixes
Chores