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

Update for latest wkt #221

Merged
merged 6 commits into from
Aug 16, 2024
Merged

Conversation

kylebarron
Copy link
Member

@kylebarron kylebarron commented Jul 23, 2024

No description provided.

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

seems reasonable

Cargo.toml Outdated Show resolved Hide resolved
@kylebarron kylebarron marked this pull request as ready for review July 25, 2024 21:11
@kylebarron kylebarron requested review from michaelkirk and nyurik July 25, 2024 21:11
Cargo.toml Outdated Show resolved Hide resolved
}

/// Process WKT geometry
fn process_wkt_geom<P: GeomProcessor>(geometry: &Geometry<f64>, processor: &mut P) -> Result<()> {
fn process_wkt_geom<P: GeomProcessor>(geometry: &wkt::Wkt<f64>, processor: &mut P) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

can we simplify all these with use wkt::Wkt ?

Copy link
Member

Choose a reason for hiding this comment

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

there are already other entities in the local module scope called Wkt. You could use an alias, but it's unlikely to be any more concise than wkt::Wkt.

geozero/src/wkt/wkt_reader.rs Show resolved Hide resolved
geozero/src/wkt/wkt_reader.rs Show resolved Hide resolved
@kylebarron kylebarron requested a review from nyurik July 29, 2024 13:39
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

thx!!

@nyurik
Copy link
Member

nyurik commented Aug 15, 2024

When merged, will all the minor back and forth commits go into the main tree?

@kylebarron
Copy link
Member Author

I usually always squash commits when merging myself

@michaelkirk michaelkirk added this pull request to the merge queue Aug 15, 2024
@michaelkirk
Copy link
Member

I switched the merge queue to use squash commits just now.

For the record, as a staunch "rebase and merge-commit"-er, I find squashing barbaric, but barbarians have proven themselves very effective over the course of history. 😉

Another maintainer can LMK if they care. I'm open to undoing that change, but figured I'd rather be transparent about the change and ask for foregiveness than have a drawn out debate beforehand.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 15, 2024
@michaelkirk
Copy link
Member

Looks like new clippy is hitting new lint. I'll follow up with a fix.

@nyurik
Copy link
Member

nyurik commented Aug 15, 2024

I wonder if clippy should be a non voting part of ci? Ie ok to merge, but show up as an error.

-- your friendly barbarian

@michaelkirk
Copy link
Member

I wonder if clippy should be a non voting part of ci? Ie ok to merge, but show up as an error.

I'm slightly in favor of that change, though don't feel strongly.

Obviously this opens us up the risk of ignoring important clippy lints, but on balance, I'd support that change.

It's disruptive that I frequently lose part of my day trying to move something along. Often it's just 5 minutes, but it's not uncommon for it to be 20 or more. It's especially annoying because IMO 80% of the time the clippy changes are neutral, 10% of the time they are dumb, and 10% of the time they are super helpful.

To be clear, I'm pro-clippy. It's saved my bacon many times. I just don't like something with such a low signal to noise ratio scheduling my day for me.

@michaelkirk michaelkirk added this pull request to the merge queue Aug 16, 2024
Merged via the queue into georust:main with commit 9645970 Aug 16, 2024
1 check passed
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.

3 participants