Merge wkt repository with history as /wkt dir #785

nyurik · 2022-03-19T00:21:22Z

If we agree to merge this, the wkt repo should be archived, and all issues should be moved to this repo. I believe it would be good to merge it here because:

geo-test-fixtures requires wkt, and wkt requires geo-types - creating a circular dependency between two repos, making development more complicated and error-prone
this is a commonly used geo feature
this is a pure Rust lib

This merges the entire history of the wkt repository from

https://github.com/georust/wkt/tree/dbd80f22099fc11ad69a1d072c3516f9a1ca4376

as a /wkt directory, and adds the CI and adds path links from Crates.toml files:

wkt/Cargo.toml - link to /geo-types
geo-test-fixtures/Cargo.toml - link to /wkt

I agree to follow the project's code of conduct.
~~- [ ] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.~~

79: BREAKING: move `Wkt::from_str` to `FromStr` trait r=urschrei a=nyurik Most of the user's code will stay the same, but will require `use std::str::FromStr;` See also discussion in georust#77 - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Yuri Astrakhan <[email protected]>

In order to auto-catch bugs and code quality, adding clippy and fmt to CI (same as geo)

80: Fix bench build bug, added CI lints r=lnicola a=nyurik In order to auto-catch bugs and code quality, adding clippy and fmt to CI (same as geo) - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - ~[ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.~ --- Co-authored-by: Yuri Astrakhan <[email protected]>

77: Prepare for 0.10.0 release r=urschrei a=urschrei - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Stephan Hügel <[email protected]>

81: Remove authors manifest field r=nyurik,urschrei a=lnicola - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- r? `@nyurik` Co-authored-by: Laurențiu Nicola <[email protected]>

urschrei · 2022-03-19T00:22:50Z

Why do you want to do this?

nyurik · 2022-03-19T00:28:00Z

@urschrei I updated the top comment with the explanation -- mostly because there is a circular dependency between geo and wkt repos, and developing them in sync is becoming complicated. Currently i have to maintain a separate wkt fork just to get my changes to the geo repo to compile (which obviously cannot be merged as is, so may require some more contortions in the future)

This merges the entire history of the wkt repository from https://github.com/georust/wkt/tree/dbd80f22099fc11ad69a1d072c3516f9a1ca4376 as a `/wkt` directory, and adds the CI and adds path links from Crates.toml files: * wkt/Cargo.toml - link to `/geo-types` * geo-test-fixtures/Cargo.toml - link to `/wkt`

frewsxcv · 2022-03-22T00:16:58Z

How does moving the wkt crate into this repository solve the circular dependency problem?

I'd consider this to be a rather large change to this repository and the wkt repository. In the future, can you open a GitHub Issue or start a GitHub Discussion so we can discuss whether we even want them before reviewing any code?

nyurik · 2022-03-22T00:29:27Z

@frewsxcv sure thing, will do. I thought that since PR and issues are nearly identical in terms of discussion, and it was fairly simple to create this PR to see the code changes if we wanted to, while we can discuss it as if it was an issue.

Moving WKT does not solve circular dependency. What it does is it brings all components of the circular dependency into a single repo with a single CI. When a breaking change must be made to both modules, it can be done with a single PR.

Having two repos forces us to go through this process:

create a PR for geo repo
ignore geo CI errors and merge + publish it
create a PR for wkt
now wkt should pass CI since geo is already published, so merge+publish
expect the geo to start passing CI again

P.S. alternatively we can always use wkt = { git = "https://github.com/georust/wkt", branch = "some-fork" } which is kinda ugly. Note that it should not be a revision because revision implies part of the main tree, whereas wkt change must not be merged until geo is merged. This is workable if you really don't think wkt should not be part of the core libs, but would mean merging geo that relies on non-merged wkt.

rmanoka · 2022-03-22T02:57:57Z

@nyurik can you explain this part more?

ignore geo CI errors and merge + publish it

Why ignore CI?

nyurik · 2022-03-22T04:00:16Z

@nyurik can you explain this part more?

ignore geo CI errors and merge + publish it
Why ignore CI?

There are two types of changes: those that can be done in steps, e.g. merge and release #786 that introduces MultiPolygon::new() and similar. Once published, wkt repo can be changed to use the new methods, another release of wkt, and only then can geo continue with the breaking change.

There are also breaking changes which cannot IMO be done -- CI would not pass if geo repo introduces PointTZM - because the matching below wouldn't work

-/// assert!(matches!(my_type.geometry, Some(Point(_))));
+/// assert!(matches!(my_type.geometry, Some(PointTZM(_))));

Granted that there are very few of these, and in most cases they can be worked around, i.e. unit test in the doc can be removed, and a new wkt version would be released without that test, but this is no different than simply disabling certain parts of the CI during the migration, and re-enabling it later.

lnicola · 2022-03-22T06:11:38Z

This isn't a circular dependency. geo-test-fixtures is a dev-dependency of geo, and uses wkt without the geo-types feature. Unless I'm missing something, we should be able to keep using whatever wkt version for reading the test fixtures.

nyurik · 2022-03-22T06:24:56Z

This isn't a circular dependency. geo-test-fixtures is a dev-dependency of geo, and uses wkt without the geo-types feature. Unless I'm missing something, we should be able to keep using whatever wkt version for reading the test fixtures.

It's a circular dependency in terms of passing ci. See my comment above

rmanoka · 2022-03-22T06:47:53Z

I'm also not convinced yet, that this break CI. @nyurik can you point us to where this line you pasted above is:

assert!(matches!(my_type.geometry, Some(Point(_))));

I'm inclined against this PR, and in general pro-actively merging repos. If anything, we should see a couple of PRs that do break CI due to circular deps, and try out the alternative / more cumbersome workflow before taking a call on this (imho).

lnicola · 2022-03-22T08:28:10Z

It's a circular dependency in terms of passing ci. See my comment above

I don't understand -- why does wkt care about how to instantiate MultiPolygon? It depends on an older version of geo-types (without new) and that code is feature-gated anyway.

nyurik · 2022-03-22T12:04:39Z

I'm also not convinced yet, that this break CI. @nyurik can you point us to where this line you pasted above is:
assert!(matches!(my_type.geometry, Some(Point(_))));
I'm inclined against this PR, and in general pro-actively merging repos. If anything, we should see a couple of PRs that do break CI due to circular deps, and try out the alternative / more cumbersome workflow before taking a call on this (imho).

@rmanoka see https://github.com/georust/wkt/pull/85/files#diff-0fb2eeea3273a4c9b3de69ee949567f546dc8c06b1e190336870d00b54ea0979R140

I created two PRs to demonstrate the difficulty of introducing any breaking changes that affect wkt -- see #790 and georust/wkt#85

nyurik · 2022-04-05T03:16:04Z

closing for now thx for @lnicola proposed workaround - see https://github.com/georust/geo/pull/742/files#diff-d540e35931c0109d7b7d24d53436d1f2e5d883fc264fcfa22d517b8a43ec7cbb

lnicola · 2022-04-05T15:02:14Z

lnicola proposed workaround

Just to be clear, that's not the solution I proposed. My suggestion was to skip geo-types when deserializing the WKT.

nyurik · 2022-04-05T15:05:39Z

@lnicola in that case I must have misunderstood your idea - i thought you meant conversion of old geo-types to the new ones. Could you sketch a prototype of your idea?

lnicola · 2022-04-05T15:10:59Z

I can't find the comment where I explained it, but at least I'm at the computer now 😄.

The main geometry type in wkt is https://docs.rs/wkt/latest/wkt/enum.Geometry.html. So it's enough to parse the fixtures into wkt::Geometry, then convert that into geo_types::Geometry from the current repo.

But you're not doing that. You're using a wkt::Geometry to geo_types::Geometry conversion available (feature-gated, but accidentally enabled) in wkt. So you're going from wkt::Geometry to geo_types::Geometry, to a potentially different geo_types::Geometry. This also forces you to add extra cases in the conversion code, like those for line and rectangle.

frewsxcv added 30 commits December 28, 2014 12:28

Empty cargo project

680987c

Create very basic, incomplete tokenizer

66c1689

Add private utility function, DRY

bb96502

Expand tests

14839ec

Better number tokenizing, and tests!

1e38edd

Stop using deprecated from_str function

b2bd644

Simplify test asserts

b217307

Remove unused commented out function

6ae7d01

Add test for tokenizing WKT POINT

b8e18d4

Add test for empty string

b66ed72

Greatly simplify tests

91b29ed

Better error handling for number parsing

84820f5

Ignore leading '+' for numbers

0a66c7e

Beginning of Wkt struct

1fdb112

Implement POINT interpreting

2dfd0e5

Change constructor from String -> str

3a0d4ce

Constructors should return Result

950fee0

Reflect changes to String.remove

56b6d50

Empty string should be valid

9e0a835

Add tests for invalid points

6e91904

Create 'Coord' struct for base of geometries

df554ee

Create modules for each type

87841a9

Fill in error messages

85d200c

Create basic README

61b6071

Remove unnecessary mut

1462ef1

Make tokenizer peekable

206d444

Stop using deprecated method

fe40fcc

Create LineString struct

3369a82

Minor refactoring

3683e25

Handle outer parenthesis in parent call, DRY

18ee64e

bors bot and others added 8 commits February 24, 2022 18:35

Fix bench build bug, added CI lints

26f5db3

In order to auto-catch bugs and code quality, adding clippy and fmt to CI (same as geo)

Prepare for 0.10.0 release

c164a0e

Remove authors manifest field

2866e4c

move to wkt/

eecc431

nyurik force-pushed the wkt branch from 3cffb9c to ebf44f6 Compare March 19, 2022 00:26

nyurik force-pushed the wkt branch from ebf44f6 to 917fb40 Compare March 19, 2022 00:46

nyurik force-pushed the wkt branch from 917fb40 to 235d680 Compare March 19, 2022 01:07

nyurik mentioned this pull request Mar 20, 2022

Implement 3D and Measure support for geo types #742

Closed

2 tasks

nyurik closed this Apr 5, 2022

nyurik deleted the wkt branch April 5, 2022 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge wkt repository with history as /wkt dir #785

Merge wkt repository with history as /wkt dir #785

nyurik commented Mar 19, 2022 •

edited

Loading

urschrei commented Mar 19, 2022

nyurik commented Mar 19, 2022 •

edited

Loading

frewsxcv commented Mar 22, 2022

nyurik commented Mar 22, 2022 •

edited

Loading

rmanoka commented Mar 22, 2022

nyurik commented Mar 22, 2022 •

edited

Loading

lnicola commented Mar 22, 2022

nyurik commented Mar 22, 2022

rmanoka commented Mar 22, 2022

lnicola commented Mar 22, 2022

nyurik commented Mar 22, 2022

nyurik commented Apr 5, 2022

lnicola commented Apr 5, 2022 •

edited

Loading

nyurik commented Apr 5, 2022

lnicola commented Apr 5, 2022

Merge wkt repository with history as /wkt dir #785

Merge wkt repository with history as /wkt dir #785

Conversation

nyurik commented Mar 19, 2022 • edited Loading

urschrei commented Mar 19, 2022

nyurik commented Mar 19, 2022 • edited Loading

frewsxcv commented Mar 22, 2022

nyurik commented Mar 22, 2022 • edited Loading

rmanoka commented Mar 22, 2022

nyurik commented Mar 22, 2022 • edited Loading

lnicola commented Mar 22, 2022

nyurik commented Mar 22, 2022

rmanoka commented Mar 22, 2022

lnicola commented Mar 22, 2022

nyurik commented Mar 22, 2022

nyurik commented Apr 5, 2022

lnicola commented Apr 5, 2022 • edited Loading

nyurik commented Apr 5, 2022

lnicola commented Apr 5, 2022

nyurik commented Mar 19, 2022 •

edited

Loading

nyurik commented Mar 19, 2022 •

edited

Loading

nyurik commented Mar 22, 2022 •

edited

Loading

nyurik commented Mar 22, 2022 •

edited

Loading

lnicola commented Apr 5, 2022 •

edited

Loading