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

Lower MSRV to 1.65 #313

Merged
merged 8 commits into from
Dec 5, 2023
Merged

Lower MSRV to 1.65 #313

merged 8 commits into from
Dec 5, 2023

Conversation

DaniPopes
Copy link
Contributor

@DaniPopes DaniPopes commented Nov 11, 2023

Synopsis

The MSRV bump in #300 is unnecessary. The 1.72 minimum is only needed for tests, but the actual implementation depends on 1.65.

Solution

Lower MSRV from 1.72 to 1.65. Only build instead of test in MSRV CI job.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

The MSRV bump in JelteF#300 is unnecessary.
The 1.72 minimum is only needed for tests, but the actual implementation depends on 1.65.
@DaniPopes DaniPopes changed the title chore: lower MSRV to 1.65 Lower MSRV to 1.65 Nov 11, 2023
@tyranron tyranron added this to the 1.0.0 milestone Nov 13, 2023
tyranron
tyranron previously approved these changes Nov 13, 2023
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@DaniPopes thanks for taking care of the details!

@JelteF please, update the required CI jobs in project settings, and merge.

tyranron
tyranron previously approved these changes Nov 13, 2023
@tyranron
Copy link
Collaborator

ping @JelteF

@JelteF
Copy link
Owner

JelteF commented Nov 16, 2023

I think it might be nice to have a bit higher MSRV than strictly necessary, because we'll likely keep it the same for the next few years. So I'm not sure we should merge this PR

@tyranron
Copy link
Collaborator

@JelteF treating MSRV via SemVer of the crate seems to be quite uncomfortable: either we change the major version when don't really break API, or we are committed to not use newer Rust features until the real API break happens.

Considering that MSRV is not that much of the demand for end users (people tend to use newer versions of Rust), we could clearly state (in README) and separate this in the following manner:

  • Changing MSRV is treated as a minor version in terms of SemVer.
  • So, If you're not concerned about MSRV in your projects, you just use the default caret requirement: derive_more = "1", or derive_more = "1.0", or derive_more = "^1.0".
  • However, if you're concerned about MSRV in your projects, then just use the tilde requirement to omit breaks: derive_more = "~1.0", or derive_more = "~1.0.0"

@tyranron
Copy link
Collaborator

@JelteF ping

1 similar comment
@tyranron
Copy link
Collaborator

@JelteF ping

@JelteF
Copy link
Owner

JelteF commented Dec 1, 2023

I think your proposal sounds good, but we should update the readme accordingly

@tyranron tyranron marked this pull request as draft December 1, 2023 17:18
@tyranron tyranron self-requested a review December 1, 2023 17:18
@tyranron tyranron marked this pull request as ready for review December 4, 2023 16:54
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@JelteF I've added "MSRV policy" section to README.

I've also returned running cargo test in msrv CI job, because it's vital to check macro expansion too for MSRV. Anything that doesn't fit MSRV from user-side in tests, I've hidden under the #[cfg(not(msrv))].

I'll merge this tomorrow, so you can have a look if you want to.

@tyranron tyranron merged commit b0b575f into JelteF:master Dec 5, 2023
16 checks passed
@DaniPopes DaniPopes deleted the lower-msrv branch February 7, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants