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.

Sign up for GitHub

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

Jump to bottom

Remove unneeded trait lifetime in geo-traits #1114

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Remove unneeded trait lifetime in geo-traits #1114

merged 3 commits into from
Nov 27, 2023

Conversation

kylebarron
Copy link
Member

@kylebarron kylebarron commented Nov 17, 2023 •
edited
Loading

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

Change list

I've been incubating traits in geoarrow-rs, so this matches the same PR I made there: geoarrow/geoarrow-rs#253, which also has updated trait implementations on WKB objects.

kylebarron
kylebarron commented Nov 17, 2023
View reviewed changes
geo-traits/src/geometry_collection.rs
type Iter = Cloned<Iter<'a, Self::ItemType>>;
impl<'a, T: CoordNum> GeometryCollectionTrait for &'a GeometryCollection<T> {
type T = T;
type ItemType<'b> = Geometry<Self::T> where
Copy link
Member Author

@kylebarron kylebarron Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it's weird that this doesn't get modified with cargo fmt...

@kylebarron
Copy link
Member Author

it looks like ci on the traits branch was already failing for 1.63 #1019

@kylebarron kylebarron mentioned this pull request Nov 17, 2023
Draft
2 tasks
@michaelkirk
Copy link
Member

We've already bumped MSRV to 1.65 on main, so I think you'll need to manually bump MSRV for the new geo-traits for CI to pass.

@kylebarron
Copy link
Member Author

We've already bumped MSRV to 1.65 on main, so I think you'll need to manually bump MSRV for the new geo-traits for CI to pass.

I'm confused; geo-traits is already set to 1.65 I thought:

geo/geo-traits/Cargo.toml

Line 10 in e952ed1

rust-version = "1.65"

@michaelkirk
Copy link
Member

I pushed a commit. It was easier than explaining. Hope that's ok. 😅

@kylebarron
Copy link
Member Author

oh I see, thanks! 🙂

@kylebarron
Copy link
Member Author

I pushed a commit. It was easier than explaining. Hope that's ok. 😅

I'm mostly impressed you were able to push! 😄 I've never figured out how to push to an incoming PR from a branch outside the main repo, even when Allow edits and access to secrets by maintainers is turned on

@kylebarron
Copy link
Member Author

Is this good to merge? 🙂 it's onto a separate branch anyways

@frewsxcv
Copy link
Member

Yep! Go for it

@frewsxcv
Copy link
Member

Do you have merge access?

@kylebarron
Copy link
Member Author

No I don't.

@frewsxcv
Copy link
Member

How about now?

@kylebarron kylebarron merged commit f116527 into georust:traits Nov 27, 2023
18 checks passed
@kylebarron
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frewsxcv frewsxcv frewsxcv approved these changes

@michaelkirk michaelkirk michaelkirk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kylebarron @michaelkirk @frewsxcv