Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.0.0 release candidate 2 #387

Merged
merged 23 commits into from
Nov 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8524025
KML Write Function
apurvabanka Nov 7, 2024
c33c875
doc string
apurvabanka Nov 7, 2024
7ad8850
spaces
apurvabanka Nov 7, 2024
49a92e1
zip file
apurvabanka Nov 8, 2024
fb60633
Test cases
apurvabanka Nov 12, 2024
5e57936
indentation fix
apurvabanka Nov 13, 2024
dbca8ac
Merge branch 'cleder:develop' into develop
apurvabanka Nov 13, 2024
6b2cf51
requested changes
apurvabanka Nov 13, 2024
7d8867c
refactor datetime_subelement_kwarg return type for clarity
cleder Nov 13, 2024
3b070b0
Merge branch 'main' into develop
cleder Nov 13, 2024
e8302d0
bump version to 1.0.0rc2
cleder Nov 13, 2024
6a13db7
Merge branch 'develop' into develop
apurvabanka Nov 14, 2024
09f8bb8
Requested changes and deleting the output files
apurvabanka Nov 14, 2024
638a131
indentation fix
apurvabanka Nov 14, 2024
e1aa026
white spaces
apurvabanka Nov 14, 2024
e8c1fb6
refactor KmlDateTime class and add get_all_attrs utility function
cleder Nov 14, 2024
e9e4915
refactor datetime_subelement_kwarg to use try-except for error handli…
cleder Nov 14, 2024
9fe1ae9
Bump codecov/codecov-action from 4 to 5
dependabot[bot] Nov 14, 2024
ac78a7e
Merge pull request #386 from cleder/dependabot/github_actions/codecov…
cleder Nov 15, 2024
7c02d72
Merge branch 'cleder:develop' into develop
apurvabanka Nov 16, 2024
70507af
Merge pull request #378 from apurvabanka/develop
cleder Nov 16, 2024
0b6f774
refactor KML writing tests to use temporary directories for file hand…
cleder Nov 16, 2024
4cba662
refactor KML writing methods to use 'unicode' encoding and update par…
cleder Nov 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/run-all-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# run tests and lint with a variety of Python versions
---
name: Tests
on: [push, pull_request]

Check warning on line 5 in .github/workflows/run-all-tests.yml

View workflow job for this annotation

GitHub Actions / static-tests (3.12)

5:1 [truthy] truthy value should be one of [false, true]

Check warning on line 5 in .github/workflows/run-all-tests.yml

View workflow job for this annotation

GitHub Actions / static-tests (3.12)

5:1 [truthy] truthy value should be one of [false, true]

Check warning on line 5 in .github/workflows/run-all-tests.yml

View workflow job for this annotation

GitHub Actions / static-tests (3.12)

5:1 [truthy] truthy value should be one of [false, true]

jobs:
cpython:
Expand Down Expand Up @@ -46,7 +46,7 @@
pytest tests --cov=fastkml --cov=tests --cov-fail-under=95 --cov-report=xml
- name: "Upload coverage to Codecov"
if: ${{ matrix.python-version==3.11 }}
uses: codecov/codecov-action@v4
uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
verbose: true
Expand Down
2 changes: 1 addition & 1 deletion fastkml/about.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@
The only purpose of this module is to provide a version number for the package.
"""

__version__ = "1.0.0rc1"
__version__ = "1.0.0rc2"

__all__ = ["__version__"]
8 changes: 2 additions & 6 deletions fastkml/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,20 +189,16 @@ def to_string(
str,
config.etree.tostring(
element,
encoding="UTF-8",
encoding="unicode",
pretty_print=prettyprint,
).decode(
"UTF-8",
),
)
except TypeError:
return cast(
str,
config.etree.tostring(
element,
encoding="UTF-8",
).decode(
"UTF-8",
encoding="unicode",
),
)

Expand Down
21 changes: 11 additions & 10 deletions fastkml/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1092,23 +1092,24 @@ def datetime_subelement_kwarg(
kwarg: str,
classes: Tuple[Type[object], ...],
strict: bool,
) -> Dict[str, List["KmlDateTime"]]:
) -> Dict[str, "KmlDateTime"]:
"""Extract a KML datetime from a subelement of an XML element."""
cls = classes[0]
node = element.find(f"{ns}{node_name}")
if node is None:
return {}
node_text = node.text.strip() if node.text else ""
if node_text:
if kdt := cls.parse(node_text): # type: ignore[attr-defined]
return {kwarg: kdt}
handle_error(
error=ValueError(f"Invalid DateTime value: {node_text}"),
strict=strict,
element=element,
node=node,
expected="DateTime",
)
try:
return {kwarg: cls.parse(node_text)} # type: ignore[attr-defined]
except ValueError as exc:
handle_error(
error=exc,
strict=strict,
element=element,
node=node,
expected="DateTime",
)
return {}


Expand Down
43 changes: 43 additions & 0 deletions fastkml/kml.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"""

import logging
import zipfile
from pathlib import Path
from typing import IO
from typing import Any
Expand Down Expand Up @@ -244,6 +245,48 @@ def parse(
element=root,
)

def write(
self,
file_path: Path,
*,
prettyprint: bool = True,
precision: Optional[int] = None,
verbosity: Verbosity = Verbosity.normal,
) -> None:
"""
Write KML to a file.

Args:
----
file_path: The file name where to save the file.
Can be any string value
prettyprint : bool, default=True
Whether to pretty print the XML.
precision (Optional[int]): The precision used for floating-point values.
verbosity (Verbosity): The verbosity level for generating the KML element.

"""
element = self.etree_element(precision=precision, verbosity=verbosity)

try:
xml_content = config.etree.tostring(
element,
encoding="unicode",
pretty_print=prettyprint,
)
except TypeError:
xml_content = config.etree.tostring(
element,
encoding="unicode",
)

if file_path.suffix == ".kmz":
with zipfile.ZipFile(file_path, "w", zipfile.ZIP_DEFLATED) as kmz:
kmz.writestr("doc.kml", xml_content)
else:
with file_path.open("w", encoding="UTF-8") as file:
file.write(xml_content)

Comment on lines +271 to +289
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for file operations

The file operations could fail for various reasons (permissions, disk space, etc.). Consider adding appropriate error handling.

     try:
         xml_content = config.etree.tostring(
             element,
             encoding="unicode",
             pretty_print=prettyprint,
         )
     except TypeError:
         xml_content = config.etree.tostring(
             element,
             encoding="unicode",
         )
 
     if file_path.suffix == ".kmz":
-        with zipfile.ZipFile(file_path, "w", zipfile.ZIP_DEFLATED) as kmz:
-            kmz.writestr("doc.kml", xml_content)
+        try:
+            with zipfile.ZipFile(file_path, "w", zipfile.ZIP_DEFLATED) as kmz:
+                kmz.writestr("doc.kml", xml_content)
+        except (OSError, zipfile.BadZipFile) as e:
+            raise IOError(f"Failed to write KMZ file: {e}") from e
     else:
-        with file_path.open("w", encoding="UTF-8") as file:
-            file.write(xml_content)
+        try:
+            with file_path.open("w", encoding="UTF-8") as file:
+                file.write(xml_content)
+        except OSError as e:
+            raise IOError(f"Failed to write KML file: {e}") from e
📝 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.

Suggested change
try:
xml_content = config.etree.tostring(
element,
encoding="unicode",
pretty_print=prettyprint,
)
except TypeError:
xml_content = config.etree.tostring(
element,
encoding="unicode",
)
if file_path.suffix == ".kmz":
with zipfile.ZipFile(file_path, "w", zipfile.ZIP_DEFLATED) as kmz:
kmz.writestr("doc.kml", xml_content)
else:
with file_path.open("w", encoding="UTF-8") as file:
file.write(xml_content)
try:
xml_content = config.etree.tostring(
element,
encoding="unicode",
pretty_print=prettyprint,
)
except TypeError:
xml_content = config.etree.tostring(
element,
encoding="unicode",
)
if file_path.suffix == ".kmz":
try:
with zipfile.ZipFile(file_path, "w", zipfile.ZIP_DEFLATED) as kmz:
kmz.writestr("doc.kml", xml_content)
except (OSError, zipfile.BadZipFile) as e:
raise IOError(f"Failed to write KMZ file: {e}") from e
else:
try:
with file_path.open("w", encoding="UTF-8") as file:
file.write(xml_content)
except OSError as e:
raise IOError(f"Failed to write KML file: {e}") from e


registry.register(
KML,
Expand Down
8 changes: 3 additions & 5 deletions fastkml/times.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@

# regular expression to parse a gYearMonth string
# year and month may be separated by an optional dash
# year is always 4 digits, month is always 2 digits
# year is always 4 digits, month, day is always 2 digits
year_month_day = re.compile(
r"^(?P<year>\d{4})(?:-)?(?P<month>\d{2})?(?:-)?(?P<day>\d{2})?$",
)
Expand Down Expand Up @@ -205,10 +205,8 @@ def parse(cls, datestr: str) -> Optional["KmlDateTime"]:
resolution = DateTimeResolution.year_month
if year_month_day_match.group("month") is None:
resolution = DateTimeResolution.year
elif len(datestr) > 10: # noqa: PLR2004
dt = arrow.get(datestr).datetime
resolution = DateTimeResolution.datetime
return cls(dt, resolution) if dt else None
return cls(dt, resolution)
return cls(arrow.get(datestr).datetime, DateTimeResolution.datetime)
Comment on lines +208 to +209
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance error handling for date parsing

The current implementation might mask invalid input formats by attempting to parse them with arrow.get() without proper validation. Since this is a release candidate, we should ensure robust error handling.

Consider implementing more explicit error handling:

-            return cls(dt, resolution)
-        return cls(arrow.get(datestr).datetime, DateTimeResolution.datetime)
+            return cls(dt, resolution)
+        try:
+            return cls(arrow.get(datestr).datetime, DateTimeResolution.datetime)
+        except (arrow.parser.ParserError, ValueError) as e:
+            raise ValueError(
+                f"Invalid date format: {datestr}. Expected formats: "
+                "YYYY, YYYY-MM, YYYY-MM-DD, or ISO 8601 datetime"
+            ) from e
📝 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.

Suggested change
return cls(dt, resolution)
return cls(arrow.get(datestr).datetime, DateTimeResolution.datetime)
return cls(dt, resolution)
try:
return cls(arrow.get(datestr).datetime, DateTimeResolution.datetime)
except (arrow.parser.ParserError, ValueError) as e:
raise ValueError(
f"Invalid date format: {datestr}. Expected formats: "
"YYYY, YYYY-MM, YYYY-MM-DD, or ISO 8601 datetime"
) from e


@classmethod
def get_ns_id(cls) -> str:
Expand Down
40 changes: 29 additions & 11 deletions fastkml/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,32 @@ def has_attribute_values(obj: object, **kwargs: Any) -> bool:
return False


def get_all_attrs(obj: object) -> Generator[object, None, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (complexity): Consider simplifying the nested error handling and iteration logic in get_all_attrs() to improve code clarity.

The current implementation of get_all_attrs() adds unnecessary complexity with nested try-except blocks and multiple layers of iteration. Here's a simpler version that maintains separation of concerns while being more readable:

def get_all_attrs(obj: object) -> Generator[object, None, None]:
    """Get all attributes of an object."""
    try:
        for attr_name in (a for a in obj.__dict__ if not attr.startswith("_")):
            attr = getattr(obj, attr_name)
            if hasattr(attr, '__iter__') and not isinstance(attr, (str, bytes)):
                yield from attr
            else:
                yield attr
    except AttributeError:
        return

This simplified version:

  1. Combines error handling into a single try-except block
  2. Uses a single iteration with a clear condition for handling iterables
  3. Avoids catching TypeError and instead explicitly checks for iterability
  4. Excludes string types which are iterable but should be treated as values

The find_all() function can then use this cleaner implementation while maintaining the same functionality.

"""
Get all attributes of an object.

Args:
----
obj: The object to get attributes from.

Returns:
-------
An iterable of all attributes of the object or, if the attribute itself is
iterable, iterate over the attribute values.

"""
try:
attrs = (attr for attr in obj.__dict__ if not attr.startswith("_"))
except AttributeError:
return
for attr_name in attrs:
attr = getattr(obj, attr_name)
try:
yield from attr
except TypeError:
yield attr

Comment on lines +33 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing attribute access safety and handling.

While the implementation is generally sound, there are a few considerations for a 1.0.0 release:

  1. The function silently returns on AttributeError, which might mask legitimate issues
  2. It may miss attributes defined via @property decorators or slots
  3. Direct __dict__ access could expose internal state

Consider this more robust implementation:

 def get_all_attrs(obj: object) -> Generator[object, None, None]:
     """
     Get all attributes of an object.
 
     Args:
     ----
         obj: The object to get attributes from.
 
     Returns:
     -------
         An iterable of all attributes of the object or, if the attribute itself is
             iterable, iterate over the attribute values.
 
     """
+    # Get both regular attributes and properties
+    attrs = set()
     try:
-        attrs = (attr for attr in obj.__dict__ if not attr.startswith("_"))
+        attrs.update(name for name in obj.__dict__ if not name.startswith("_"))
     except AttributeError:
-        return
-    for attr_name in attrs:
+        pass
+    
+    # Include properties and descriptors from class
+    attrs.update(
+        name for name in dir(obj.__class__)
+        if not name.startswith("_") and isinstance(
+            getattr(obj.__class__, name, None),
+            property
+        )
+    )
+
+    for attr_name in attrs:
         attr = getattr(obj, attr_name)
         try:
             yield from attr
         except TypeError:
             yield attr
📝 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.

Suggested change
def get_all_attrs(obj: object) -> Generator[object, None, None]:
"""
Get all attributes of an object.
Args:
----
obj: The object to get attributes from.
Returns:
-------
An iterable of all attributes of the object or, if the attribute itself is
iterable, iterate over the attribute values.
"""
try:
attrs = (attr for attr in obj.__dict__ if not attr.startswith("_"))
except AttributeError:
return
for attr_name in attrs:
attr = getattr(obj, attr_name)
try:
yield from attr
except TypeError:
yield attr
def get_all_attrs(obj: object) -> Generator[object, None, None]:
"""
Get all attributes of an object.
Args:
----
obj: The object to get attributes from.
Returns:
-------
An iterable of all attributes of the object or, if the attribute itself is
iterable, iterate over the attribute values.
"""
# Get both regular attributes and properties
attrs = set()
try:
attrs.update(name for name in obj.__dict__ if not name.startswith("_"))
except AttributeError:
pass
# Include properties and descriptors from class
attrs.update(
name for name in dir(obj.__class__)
if not name.startswith("_") and isinstance(
getattr(obj.__class__, name, None),
property
)
)
for attr_name in attrs:
attr = getattr(obj, attr_name)
try:
yield from attr
except TypeError:
yield attr


def find_all(
obj: object,
*,
Expand All @@ -55,17 +81,9 @@ def find_all(
**kwargs,
):
yield obj
try:
attrs = (attr for attr in obj.__dict__ if not attr.startswith("_"))
except AttributeError:
return
for attr_name in attrs:
attr = getattr(obj, attr_name)
try:
for item in attr:
yield from find_all(item, of_type=of_type, **kwargs)
except TypeError:
yield from find_all(attr, of_type=of_type, **kwargs)

for attr in get_all_attrs(obj):
yield from find_all(attr, of_type=of_type, **kwargs)


def find(
Expand Down
6 changes: 2 additions & 4 deletions tests/atom_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ class TestStdLibrary(StdLibrary):
"""Test with the standard library."""

def test_atom_link_ns(self) -> None:
ns = "{http://www.opengis.net/kml/2.2}"
link = atom.Link(ns=ns)
assert link.ns == ns
link = atom.Link()
assert link.to_string().startswith(
'<kml:link xmlns:kml="http://www.opengis.net/kml/2.2"',
'<atom:link xmlns:atom="http://www.w3.org/2005/Atom"',
)

def test_atom_link(self) -> None:
Expand Down
Loading
Loading