proj w/o geo #41

michaelkirk · 2020-09-08T16:44:01Z

It seems like the potential users of proj would include people who don't need/want to opt into the whole geo ecosystem.

What would you think about having the proj interface be trait-based and having the geo-integration be a feature?

If this seems like a reasonable direction, before merging I'd like to update the docs to show:

how you can use this with the greater geo ecosystem
how you can impl your own point type if you're not using geo.
how you can use the built-in (Float, Float) impl

WDYT @urschrei?

michaelkirk · 2020-09-08T16:47:43Z

src/geo_types.rs

+/// assert_approx_eq!(result.x, 1450880.29f64, 1.0e-2);
+/// assert_approx_eq!(result.y, 1141263.01f64, 1.0e-2);
+/// ```
+impl<T: crate::proj::CoordinateType> crate::Point<T> for geo_types::Coordinate<T> {


Note - this PR would also let you do Proj stuff with Coords, not just points.

Thinking about it a bit more - we could take this even further and have a Projectable trait or somesuch that just required implementing a Iter<Item=proj::Point<T>>, then we could get easy projection of all the geo-type's. 🤔

I'm not going to pursue the Iter scenario in this PR, but would like to do it in some followup work.

michaelkirk · 2020-09-08T16:55:29Z

We could make the geo-types feature a default feature to make this a non-breaking change.

urschrei · 2020-09-09T12:27:00Z

I'm in favour of this so long as we write clear documentation around it. In general, I find it tricky to present feature interaction in a clear and concise way, but we needn't let that stop us…

michaelkirk

Ok, with the caveat of the README (see comment) I think this is ready for review. Can you take a look @urschrei?

michaelkirk · 2020-09-11T21:56:17Z

README.md

@@ -22,10 +22,7 @@ By default, this crate depends on a pre-built `libproj`, so PROJ v7.1.x must be

 ## Convert from [NAD 83 US Survey Feet](https://epsg.io/2230) to [NAD 83 Meters](https://epsg.io/26946) Using EPSG Codes
 ```rust


The README contains lots of snippets from the primary docs. Since it can be tedious and error prone to copy changes back and forth between them, I was hoping we could review just the primary docs, and once they're in good shape I can update the README before a final review.

urschrei

This lgtm, let's merge!

urschrei · 2020-10-08T11:38:48Z

I think this probably just needs a rebase now that we've switched to GH Actions?

It more closely corresponds with how we're using the terminology in geo, and paves the way for interacting with "a bunch of coords" from geo-types e.g. if we're projecting a linestring, we'd operate on the linestrings coords, not it's points. Not a big deal, but since we're introducing a new thing (the trait), no reason to inherit the old name if another fits better.

These changes were all made in src/lib.rs Then I ran: cargo readme > README.md But unfortunately that doesn't capture feature-gated docs, so manually copied over the geo-types doc

michaelkirk · 2020-10-08T17:50:44Z

Ok! Rebased and addressed conflicts in the docs.

Note that I'm (almost) wholesale copying the contents of README.md from the doc comments in src/lib.rs - the only divergence is the GH actions badge.

michaelkirk · 2020-10-08T17:51:18Z

bors try

michaelkirk · 2020-10-08T17:54:42Z

Cargo.toml

@@ -14,7 +14,7 @@ edition = "2018"

 [dependencies]
 proj-sys = { version = "0.18.3", path = "proj-sys" }
-geo-types ="0.6.0"
+geo-types = { version = "0.6", optional = true }


I snuck this in - there wasn't a reason for us to peg the patch version of geo-types was there?

michaelkirk · 2020-10-08T18:18:26Z

bors try

I'm not sure why - but bors crashed... let's try again.

bors · 2020-10-08T18:18:28Z

try

Already running a review

michaelkirk · 2020-10-08T19:47:50Z

bors r=urschrei

michaelkirk · 2020-10-08T19:49:48Z

bors cancel

bors · 2020-10-08T19:54:45Z

Build succeeded:

ci result

michaelkirk · 2020-10-08T19:55:48Z

🤦 booooorrrsss

Oh well, I just wanted to fix a little typo in the README and squash the commits, but bors will do what bors wants.

michaelkirk · 2020-10-08T19:56:29Z

Oh, actually that was my bad. I should have said bors r- instead of bors cancel. Sorry for doubting you bors 😬.

urschrei · 2020-10-08T19:58:43Z

We have always known bots and people couldn't live in harmony. It's been a good run. (Thank you for doing this!)

bors · 2021-01-06T17:59:17Z

try

Merge conflict.

michaelkirk · 2021-01-06T18:04:57Z

wth bors

michaelkirk · 2021-01-06T18:05:44Z

That eye. So piercing.

urschrei · 2021-01-06T18:06:50Z

I feel judged.

michaelkirk force-pushed the mkirk/trait branch 2 times, most recently from 39c4381 to ae08ea1 Compare September 8, 2020 16:46

michaelkirk commented Sep 8, 2020

View reviewed changes

michaelkirk force-pushed the mkirk/trait branch 2 times, most recently from d6cef04 to c0f7f71 Compare September 11, 2020 21:46

michaelkirk commented Sep 11, 2020

View reviewed changes

urschrei approved these changes Sep 27, 2020

View reviewed changes

michaelkirk force-pushed the mkirk/trait branch 3 times, most recently from 8e556ad to ee4d7db Compare September 28, 2020 19:08

michaelkirk mentioned this pull request Sep 30, 2020

include pre-compiled headers, bindgen only as optional feature #44

Open

michaelkirk added 7 commits October 8, 2020 10:08

Remove geo_types as mandatory dependency

a436012

optional geo-types integration

e64ec71

cargo fmt

78bd9b5

update docs

95db50d

Incorporate examples, rework docs, regen README

05ef865

These changes were all made in src/lib.rs Then I ran: cargo readme > README.md But unfortunately that doesn't capture feature-gated docs, so manually copied over the geo-types doc

add feature for docs.rs

cf7994b

michaelkirk force-pushed the mkirk/trait branch from ee4d7db to cf7994b Compare October 8, 2020 17:40

fix doc link to proj-sys, consistent formatting

3b66048

michaelkirk force-pushed the mkirk/trait branch from 5849f35 to 3b66048 Compare October 8, 2020 17:48

michaelkirk marked this pull request as ready for review October 8, 2020 17:50

michaelkirk commented Oct 8, 2020

View reviewed changes

fixup doc test for geo-types feature

6658684

michaelkirk changed the title ~~Draft: proj w/o geo~~ proj w/o geo Oct 8, 2020

michaelkirk mentioned this pull request Oct 8, 2020

dupe: proj w/o geo (trying to make bors happy) #49

Closed

fixup! optional geo-types integration

9f476ca

bors bot merged commit 4b03fcf into georust:master Oct 8, 2020

michaelkirk mentioned this pull request Jan 11, 2021

Interaction with Geo types georust/robust#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proj w/o geo #41

proj w/o geo #41

michaelkirk commented Sep 8, 2020

michaelkirk Sep 8, 2020

michaelkirk Sep 8, 2020 •

edited

Loading

michaelkirk Sep 11, 2020

michaelkirk commented Sep 8, 2020

urschrei commented Sep 9, 2020

michaelkirk left a comment •

edited

Loading

michaelkirk Sep 11, 2020

urschrei left a comment

urschrei commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk Oct 8, 2020

michaelkirk commented Oct 8, 2020

bors bot commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

bors bot commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

urschrei commented Oct 8, 2020

bors bot commented Jan 6, 2021

michaelkirk commented Jan 6, 2021

michaelkirk commented Jan 6, 2021

urschrei commented Jan 6, 2021

		@@ -22,10 +22,7 @@ By default, this crate depends on a pre-built `libproj`, so PROJ v7.1.x must be

		## Convert from [NAD 83 US Survey Feet](https://epsg.io/2230) to [NAD 83 Meters](https://epsg.io/26946) Using EPSG Codes
		```rust

proj w/o geo #41

proj w/o geo #41

Conversation

michaelkirk commented Sep 8, 2020

michaelkirk Sep 8, 2020

Choose a reason for hiding this comment

michaelkirk Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

michaelkirk Sep 11, 2020

Choose a reason for hiding this comment

michaelkirk commented Sep 8, 2020

urschrei commented Sep 9, 2020

michaelkirk left a comment • edited Loading

Choose a reason for hiding this comment

michaelkirk Sep 11, 2020

Choose a reason for hiding this comment

urschrei left a comment

Choose a reason for hiding this comment

urschrei commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk Oct 8, 2020

Choose a reason for hiding this comment

michaelkirk commented Oct 8, 2020

bors bot commented Oct 8, 2020

try

michaelkirk commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

bors bot commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

michaelkirk commented Oct 8, 2020

urschrei commented Oct 8, 2020

bors bot commented Jan 6, 2021

try

michaelkirk commented Jan 6, 2021

michaelkirk commented Jan 6, 2021

urschrei commented Jan 6, 2021

michaelkirk Sep 8, 2020 •

edited

Loading

michaelkirk left a comment •

edited

Loading