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

Bump min s2geography version to 0.2.0 #81

Merged
merged 9 commits into from
Dec 5, 2024

Conversation

benbovy
Copy link
Owner

@benbovy benbovy commented Dec 2, 2024

@jorisvandenbossche do you see any good reason to maintain compatibility with s2geography <0.2.0?

If not, I can remove the special cases (#if / #endif pre-processor blocks) for s2geography < 0.2.0 and I can move forward here with some refactoring of core functionality (e.g., clone s2geography objects) that would otherwise be cumbersome to make if we want to maintain compatibility with older s2geography versions.

@martinfleis
Copy link
Contributor

Given spherely does not have any release compatible with 0.1.0 I don't see a reason to promise any backwards compatibility at this point.

@benbovy
Copy link
Owner Author

benbovy commented Dec 3, 2024

Yes I agree. I also think it will be unlikely to have s2geography as a common dependency of multiple downstream packages in the same environment so maintaining backwards compatibility probably won't be very useful anyway except for development iterations, keeping CI running, etc. between the releases.

@jorisvandenbossche
Copy link
Collaborator

Yep, fully agreed just bumping the minimum version, and cleaning up the code.
Maybe you can just keep the machinery in cmakelists.txt to provide the version to be able to have those #if / #endif pre-processor blocks (we might use those in the near future again for temporary using things from s2geography 0.3dev until we do a new release)

@jorisvandenbossche
Copy link
Collaborator

You can also remove some skips in the tests, eg

needs_s2geography_0_2 = pytest.mark.skipif(
Version(spherely.__s2geography_version__) < Version("0.2.0"),
reason="Needs s2geography >= 0.2.0",
)

@benbovy
Copy link
Owner Author

benbovy commented Dec 3, 2024

This PR is ready for review. I'll implement new spherely features on top of s2geography 0.2.0 in follow-up PRs.

benbovy added a commit to paleolimbot/s2geography that referenced this pull request Dec 3, 2024
For more context:

- https://github.com/paleolimbot/s2geography/pull/32/files#r1865998806
- #22 (original PR)

I'm not sure exactly what would be a proper fix, but at least this
solves Spherely package builds
(benbovy/spherely#81) and hopefully doesn't
introduce regressions in other build configurations.
@benbovy benbovy merged commit 3f58302 into main Dec 5, 2024
21 checks passed
@benbovy benbovy deleted the bump-min-s2geography-version-020 branch December 5, 2024 12:12
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