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

Migrate to 2021 edition and add examples #71

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Migrate to 2021 edition and add examples #71

merged 1 commit into from
Feb 24, 2022

Conversation

urschrei
Copy link
Member

  • 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.

@lnicola
Copy link
Member

lnicola commented Jan 19, 2022

We could go right to 2021, I think?

@urschrei
Copy link
Member Author

Oh, I was just being conservative because nothing else in georust is 2021 yet

@lnicola
Copy link
Member

lnicola commented Jan 19, 2022

Might be a good place to start, since wkt isn't tested on older compilers 😄.

@urschrei urschrei force-pushed the examples branch 3 times, most recently from 67f40e4 to 550ec59 Compare January 19, 2022 11:06
@urschrei urschrei changed the title Migrate to 2018 edition and add examples Migrate to 2021 edition and add examples Jan 19, 2022
@urschrei
Copy link
Member Author

Well I guess we know it doesn't work.

@lnicola
Copy link
Member

lnicola commented Jan 19, 2022

We need something like:

    - name: Install Rust toolchain
      uses: actions-rs/toolchain@v1
      with:
        toolchain: stable
        profile: minimal
        override: true
        components: rustfmt, rust-src

@urschrei
Copy link
Member Author

urschrei commented Jan 19, 2022

Should I add a doc entry for the serde feature? Is it in use? I didn't know WKT-in-JSON was something people did.

nm I did it.

@urschrei
Copy link
Member Author

urschrei commented Jan 19, 2022

Two last things I'd like to nail down:

  • a one-line doc comment for the Wkt struct (what's it for?)
  • a one-line doc comment for the Geometry enum (what's it for?)

- run: cargo install cargo-all-features
- run: cargo build-all-features
- run: cargo test-all-features
- run: cargo test
Copy link
Member

Choose a reason for hiding this comment

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

Was it too slow to run all the feature combinations?

I think we should do at least cargo test --all-features and cargo test --no-default-features

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, although no-default-features will cause our examples to fail…

Copy link
Member

Choose a reason for hiding this comment

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

I think there's a way to declare required features for examples. Testing with --no-default-features still seems desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, trying to find the details now instead of putting cfgs everywhere

Copy link
Member

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/61417700 doesn't look great, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nooo

src/lib.rs Outdated
//! use std::convert::TryInto;
//! use wkt::Wkt;
//! use geo_types::Point;
//! // point's items field contains a wkt::Geometry enum member
Copy link
Member

Choose a reason for hiding this comment

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

Yeah... items is weird. I guess the idea is that you can express multiple geometries without explicitly putting them into a GeometryCollection?

I tried to track down the corresponding section in the spec, but chickened out after a few minutes.

http://docs.opengeospatial.org/is/18-010r7/18-010r7.html

Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to not even mention items in the introductory example, and only show it in the case where there are multiple items? Otherwise is there any reason to even know about items?

Copy link
Member

Choose a reason for hiding this comment

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

I guess without the geo-types feature, items is the only way to get the geometry =(

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be great to know whether anyone actually uses the multiple geometry functionality in the wild…

Copy link
Member

@michaelkirk michaelkirk 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 adding some WKT docs! I agree it's unnecessarily difficult to get started with the crate without them.

None of my feedback should be considered blocking.

@nyurik
Copy link
Member

nyurik commented Feb 24, 2022

@urschrei should this be merged? There are two approvals, but no merge, so high chance of bitrot

@urschrei
Copy link
Member Author

Oh, yeah. I think I was just leaving it for a few days.

@urschrei
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 24, 2022

Build succeeded:

@bors bors bot merged commit a1f0ada into master Feb 24, 2022
@urschrei urschrei deleted the examples branch February 24, 2022 16:05
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.

4 participants