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

Remove use of const fn, add CI tests for MSRV #481

Merged
merged 1 commit into from Sep 20, 2022
Merged

Conversation

dralley
Copy link
Collaborator

@dralley dralley commented Sep 18, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2022

Codecov Report

Merging #481 (b8cdc1f) into master (da5f1e7) will decrease coverage by 0.14%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
- Coverage   54.17%   54.03%   -0.15%     
==========================================
  Files          30       30              
  Lines       12640    12662      +22     
==========================================
- Hits         6848     6842       -6     
- Misses       5792     5820      +28     
Flag Coverage Δ
unittests 54.03% <93.10%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/name.rs 88.54% <75.00%> (ø)
src/reader/mod.rs 92.04% <83.33%> (ø)
src/de/escape.rs 21.05% <100.00%> (ø)
src/de/simple_type.rs 90.63% <100.00%> (ø)
src/encoding.rs 100.00% <100.00%> (ø)
src/escapei.rs 12.15% <100.00%> (-0.07%) ⬇️
src/events/attributes.rs 93.90% <100.00%> (ø)
src/events/mod.rs 72.88% <100.00%> (ø)
src/reader/parser.rs 98.62% <100.00%> (-0.69%) ⬇️
src/reader/slice_reader.rs 95.31% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dralley dralley changed the title Drop MSRV back to 1.56 by removing use of const fn, add CI tests for MSRV Drop MSRV back to 1.54 by removing use of const fn, add CI tests for MSRV Sep 18, 2022
Copy link
Contributor

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Thanks for going the extra mile and making things work even for 1.54 instead of 1.56!

Cargo.toml Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

So I think, that changes should be done:

  • fix key in Cargo.toml
  • set MSRV according to cargo check
  • add a badge to readme

Also, maybe add a test for minimal versions? It seem that proper way to test it is using https://lib.rs/crates/cargo-minimal-versions.

Specifying dependencies with caret seems unnecessary, actually, their are already specified as caret dependencies. I think, that this part can be reverted if that will not fail the build.

Also, it seems that actually not all consts are unsupported on old rustc, maybe leave some of them? I don't insist, though.

@dralley
Copy link
Collaborator Author

dralley commented Sep 19, 2022

set MSRV according to cargo check

I assume by this you mean: repeatedly downgrade the toolchain until cargo check doesn't work anymore? Or did you mean something else?

add a badge to readme

I looked around at other projects to see how it is typically done, and it actually seems rather uncommon. serde, tokio, hyper, axum, kube-rs, rustls, thiserror, opencv-rust, etc... none of them use it. I did find one for the regex crate, but looking at how the badge is configured compared to the others, I'm not sure that it's worth it.

@Mingun
Copy link
Collaborator

Mingun commented Sep 19, 2022

I assume by this you mean: repeatedly downgrade the toolchain until cargo check doesn't work anymore? Or did you mean something else?

I mean, do the same steps that you did to find MSRV as 1.54, but run cargo check instead of cargo check --all-features as, it seems, you did. You can also try https://github.com/foresterre/cargo-msrv

I did find one for the regex crate, but looking at how the badge is configured compared to the others, I'm not sure that it's worth it.

regex, serde, actix just output static version of badge (i. e. MSRV is encoded in the link) generated by shields.io. This is fine for us as well.

@dralley dralley changed the title Drop MSRV back to 1.54 by removing use of const fn, add CI tests for MSRV Remove use of const fn, add CI tests for MSRV Sep 20, 2022
It makes the MSRV overly restrictive, and doesn't provide much benefit.
Also add an MSRV check to the CI

closes tafia#479
@dralley
Copy link
Collaborator Author

dralley commented Sep 20, 2022

cargo-msrv said the minimum was 1.43.1, but I was having some issues with the MSRV check with that version due to the document-features dependency. I'm not quite sure what was going on with that.

But in any case it works fine if I bump it up a bit higher to 1.46.0.

@Mingun Mingun merged commit 240f204 into tafia:master Sep 20, 2022
@dralley dralley deleted the msrv branch September 20, 2022 12:38
@vojtechkral
Copy link

Hey guys, I ran into this, can I trouble you to tag a release with this fix? 🙏

@jonhoo
Copy link

jonhoo commented Oct 22, 2022

Gentle nudge on this @Mingun — could we get a new release with this fix too?

@Mingun
Copy link
Collaborator

Mingun commented Oct 23, 2022

Yes, I'll make a release soon. Actually, I plan to make a new release after merging #490, but because it introduces some significant changes, it will be better to make a intermediate release with features, wanted by community now. After all, releases does not happen much

0.26.0 released.

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

6 participants