-
Notifications
You must be signed in to change notification settings - Fork 37
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
Fix invalid WKT empty geometries #160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
} else { | ||
self.out.write_all(b"(")?; | ||
} | ||
Ok(()) | ||
} | ||
fn geom_end(&mut self) -> Result<()> { | ||
self.out.write_all(b")")?; | ||
if let Some(geometry_size) = self.geometry_sizes.pop() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we ever expect this to be None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A geometry would have to end before it began, so in theory no ? I just tried to get around the fact that Vec::pop
always returns an option!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding something like else { debug_assert!(false, "ended geometry that didn't start") }
as a "this should never happen but we don't want to blow up in production if it does"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch... #160 (comment)
self.geom_begin(idx, b"POINT EMPTY") | ||
// we intentionally omit calling geom_end(), because POINT EMPTY has no closing paren | ||
self.geom_begin(b"POINT", true, 0, idx)?; | ||
self.geom_end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems less weird. 👍
We have a problem with the csv If we had access to the geometry size in processor functions |
I've just opened Oreilles#3 against your own branch that I think addresses this (and simplifies my old crappy code from yesteryear!). Take a look and merge into your branch if you think it makes sense. |
I'm also open to this approach (or we could do both) but it seems like a bigger breaking change, so my preference at this point would be to avoid it. |
impl<'a, W: Write> WktWriter<'a, W> { | ||
pub fn new(out: &'a mut W) -> WktWriter<'a, W> { | ||
impl<W: Write> WktWriter<W> { | ||
pub fn new(out: W) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but wanted to highlight the change for other reviewers that we're changing WktWriter to take any impl Write
rather than only &mut impl Write
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for simplifying writters so that they own the underlying impl Write
- but should it possibly be a separate PR that changes them all? It feels like a counter-pattern to introduce it in just one writer as part of an unrelated empty geometry support. Then we can rebase this PR on top of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with doing it for all writers. Who wants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not unrelated, in that it's required to fix the bug. But yes, I agree it would be "cleaner" to switch all the API's over, rebase this PR and then merge it rather than follow up with changing the rest of the API's after this is merged. My judgement call was that it wasn't worth the extra paperwork, but if you feel differently thats fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok either way, just as long as we don't release until all writers are more-or-less lined up (and of course its a minor rather than a patch change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and of course its a minor rather than a patch change
Just to be clear, changing from &mut Write -> Write
is a non-breaking change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WktWriter
is part of the public API - if you change its signature, just like we changed writers in #155 and #150, I think all of these should be released as 0.11, or else any existing code using this in dependencies: geozero = "0.10.0"
and using MvtWriter::new()
or using let w = WktWriter::new(); w.dims.z = true;
will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a breaking change to WktWriter in this PR if that's what you mean (see the changelog entry).
So is everyone OK to merge this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
bors r+ |
Configuration problem: |
i thought there were some discussions in rust itself to stop using bors because github now covers most such usecases? |
Yes, there were some of us trying to figure out GH merge queues a couple weeks ago. If you look at the proj crate’s recent commits for example, you’ll see some of my misadventures. As far as I know we don’t have any working examples on georust where GH’s merge queues function like bors: i.e. I trigger the process, then the tests run against an up-to-date branch and only get merged into main once they pass.It seems so basic, but i kept finding that the branches would merge immediately, regardless of if the tests pass.I’m sure it’s possible, I just didn’t personally dedicate the time to figuring it out. If you tell me what series of buttons to press I can press them.
|
I just realized that this project does not, and never did, use bors. 😅 |
@michaelkirk in Martin, we have branch protection rule for
I think if you also enable |
I got it working! Ultimately I think the issue was something else, but thanks for the sanity check @nyurik. |
As of georust#160, this breaks with invalid output: GEOMETRYCOLLECTION EMPTYPOINT(-122.329051 47.6069),POINT(-122.266529 47.515984)
As of georust#160, this breaks with invalid output: GEOMETRYCOLLECTION EMPTYPOINT(-122.329051 47.6069),POINT(-122.266529 47.515984)
Currently, empty geometries would be writter in WKT as:
which isn't valid WKT.
This PR addresses this issue and output instead: