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

#849 Bump the minor version of approx. #850

Merged
merged 2 commits into from Jun 24, 2022

Conversation

martinfrances107
Copy link
Contributor


Consitently and in many Cargo.toml files - bump the minor version of approx.

-approx = "0.4.0"
+approx = "0.5.1"

@martinfrances107
Copy link
Contributor Author

just a note to myself

The force push just now was a rebase picking up the 3 recent commits to main.

@@ -19,12 +19,12 @@ use-rstar_0_8 = ["rstar_0_8", "approx"]
use-rstar_0_9 = ["rstar_0_9", "approx"]

[dependencies]
approx = { version = "0.4.0", optional = true }
approx = { version = "0.5.1", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

This would constitute a breaking change for geo-types, right?

Since geo-types is a common dependency tying together a lot of other libs, we try to be pretty conservative about when we have a semver breaking release.

What would people think about preferring something like we did with rstar here: We'd include the latest version of approx for people via an approx_0_5 feature flag, while leaving the default approx at 0.4 until the next breaking release of geo-types.

As for the other crates affected by this PR, I'm not worried about breaking semver there, as they are less often used as dependencies than geo-types.

Anyone else have an opinion?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think the rstar approach worked well and we're not really close to a geo-types release that would warrant a breaking change, as you say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would constitute a breaking change for geo-types, right?

Can I just understand the thought process here?

"Approx 0.5.1"

So one of our sub packages is making a minor number upgrade ...

Under the semi version rules as I understand them an increase in the minor number implies any features that have been added do not affect the API. So they are either modification to internal algorithms, or the addition of extra functions.

As I read it ONLY if approx had bumped them major version number - then would that be a breaking change?
Maybe I am wrong?

I am just trying to understand the rules of the road here... because as I said in #849
My interest is more than fixing approx. it is keeping the technical debt of outdated packages manageable.

I am happy to do the leg work, and maybe add other packages to the PR. Or submit a series of PR taking this or that into consideration.

I know than means lots of consultation and it is going to be a series painful steps keeping the thing updated.
but in my experience the alternative is the project becomes locked into an un-upgrade-able state
as the active contributors needs diverge..

What I mean is say group A needs the version to be on these numbers but group B need a alternate set of sub-dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Under the semi version rules as I understand them an increase in the minor number implies any features that have been added do not affect the API. So they are either modification to internal algorithms, or the addition of extra functions.

I think you are right - but only when talking about packages that are post 1.0.0 release.

Packages that are pre-1.0.0 (0.y.z) technically can break anything and everything with every release, but in practice what I see is more conservative: that minor versions (bumping the y) can (and typically do) add breaking changes while bumps to the patch (z) are semver safe.

More to the point: If we bump approx to 0.5, while other libraries that depend on us are still using approx 0.4, then cargo will no longer resolve a compatible version between the crates.

A third option occurs to me, since we're not depending on any of the changes introduced in 0.5, could we loosen up and accept a range of approx versions?

Something like:

approx = ">= 0.4.0, < 0.6.0"

I think that'd allow people to use the latest while not forcing them to upgrade, right?

@martinfrances107
Copy link
Contributor Author

but only when talking about packages that are post 1.0.0 release.

I see that now... Thank you for changing my perspective...

That does complicate things.

I am uncertain about the way forward.

approx = ">= 0.4.0, < 0.6.0"

is certainly a good option.

My only concrete sense is that we need to make the change in a unreleased, un-tagged version .. and let various parties understand the consequences of the move. but hey everyone else knew that coming into this.

@martinfrances107
Copy link
Contributor Author

I have folded in @michaelkirk more considered solution.

This may need more discussion

@michaelkirk
Copy link
Member

Thanks @martinfrances107!

I think this approach makes the most sense, but I'm going to wait a couple days before merging in case anyone has other insights.

@martinfrances107
Copy link
Contributor Author

Speaking only about the test failures,... well the deprecation warnings.

Things will be ok once @michaelkirk's patch #851 lands.

@michaelkirk
Copy link
Member

#851 is merged!

bors try

bors bot added a commit that referenced this pull request Jun 23, 2022
@bors
Copy link
Contributor

bors bot commented Jun 23, 2022

try

Build succeeded:

@michaelkirk
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Jun 24, 2022
850: #849 Bump the minor version of approx. r=michaelkirk a=martinfrances107

- [x ] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
---

Consitently and in many Cargo.toml files  - bump the minor version of approx.


-approx = "0.4.0"
+approx = "0.5.1"

857: geo/geo bump  proj to 0.27.0 r=michaelkirk a=martinfrances107

The is a ongoing discussion in #856 about how to signal to the wider community that our dependencies are changing.

But this is issue follows the pattern developed in #850 



859: bump geojson to 0.23.0 r=michaelkirk a=martinfrances107

geojson is a internal dev-dependency.

860: Hide test artifact. r=michaelkirk a=martinfrances107

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
---

Alter .gitignore to remove a test artefact.


Co-authored-by: Martin <martinfrances107@hotmail.com>
@bors
Copy link
Contributor

bors bot commented Jun 24, 2022

Build failed (retrying...):

bors bot added a commit that referenced this pull request Jun 24, 2022
850: #849 Bump the minor version of approx. r=michaelkirk a=martinfrances107

- [x ] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
---

Consitently and in many Cargo.toml files  - bump the minor version of approx.


-approx = "0.4.0"
+approx = "0.5.1"

857: geo/geo bump  proj to 0.27.0 r=michaelkirk a=martinfrances107

The is a ongoing discussion in #856 about how to signal to the wider community that our dependencies are changing.

But this is issue follows the pattern developed in #850 



Co-authored-by: Martin <martinfrances107@hotmail.com>
@bors
Copy link
Contributor

bors bot commented Jun 24, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@bors
Copy link
Contributor

bors bot commented Jun 24, 2022

Build succeeded:

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.

None yet

3 participants