Skip to content

Conversation

@cappuc
Copy link
Contributor

@cappuc cappuc commented Jul 23, 2025

  • I agree to follow the project's code of conduct.
  • I added an entry to the project's change log file if knowledge of this change could be valuable to users.
    • Usually called CHANGES.md or CHANGELOG.md
    • Prefix changelog entries for breaking changes with "BREAKING: "

This change allows the parsing of kml files with empty coordinates (this can be checked on client code):

<?xml version="1.0" encoding="utf-8"?>
<kml xmlns="http://www.opengis.net/kml/2.2">
  <Document>
    <Placemark>
      <name>test</name>
      <LineString>
        <altitudeMode>absolute</altitudeMode>
        <coordinates />
      </LineString>
    </Placemark>
  </Document>
</kml>

I have customers provided kml files with some valid Placemark and some invalid (empty coordinates like in the example).
Right now I cannot parse the file because it return an error for empty coordinates but this can be safely checked inside client code.

@cappuc cappuc changed the title Dont fail parsing on empty coordinates Don't fail parsing on empty coordinates Jul 23, 2025
@pjsier
Copy link
Member

pjsier commented Jul 24, 2025

Thanks for the PR @cappuc! Coordinates are required by the spec but I agree with you that this might be better to sort out on the client side rather than failing to parse so this makes sense.

The clippy errors are unrelated to your changes but don’t worry about those. Could you add a test for this though? Then I can take a closer look but seems like it should be good

@cappuc
Copy link
Contributor Author

cappuc commented Jul 24, 2025

Hi @pjsier,

I added a simple test that doesn't fail with a document with empty coordinates. Do you want to add more tests or is this enough?

Copy link
Member

@pjsier pjsier left a comment

Choose a reason for hiding this comment

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

Thanks for that update! I think we'll need to tweak the test but once that's updated we should be able to merge

@cappuc cappuc requested a review from pjsier July 25, 2025 13:15
Copy link
Member

@pjsier pjsier left a comment

Choose a reason for hiding this comment

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

Thanks for making that update! This looks great, will merge and cut a new release

@pjsier pjsier merged commit 5b273e5 into georust:main Jul 25, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants