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 MSRV to 1.57.0 and base64 to 0.20 #431

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Conversation

juntyr
Copy link
Member

@juntyr juntyr commented Dec 12, 2022

Supersedes #430

If we want to upgrade to base64 0.20, we need to bump our MSRV to 1.57 as well.

  • I've included my change in CHANGELOG.md

@codecov-commenter
Copy link

Codecov Report

Base: 86.88% // Head: 86.88% // No change to project coverage 👍

Coverage data is based on head (c0f1de4) compared to base (0ce76f9).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #431   +/-   ##
=======================================
  Coverage   86.88%   86.88%           
=======================================
  Files          59       59           
  Lines        7267     7267           
=======================================
  Hits         6314     6314           
  Misses        953      953           
Impacted Files Coverage Δ
src/error.rs 33.33% <0.00%> (ø)

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@juntyr juntyr requested a review from torkleyy December 13, 2022 09:48
@juntyr
Copy link
Member Author

juntyr commented Jan 9, 2023

Will need to be followed up by a migration to 0.21 (#432)

Copy link
Contributor

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

Looks good, I only wonder if base64 is good to have baked into RON, given that you could easily do it yourself with serde attributes.

Removing it would be a rather unpleasent breaking change for anyone using it however... Any thoughts?

You can merge it as-is, this is just what came to my mind right now ^^

@juntyr
Copy link
Member Author

juntyr commented Jan 10, 2023

Looks good, I only wonder if base64 is good to have baked into RON, given that you could easily do it yourself with serde attributes.

Do you mean users could hardcode how they want to (de)serialise their bytes? That's definitely true (and what I do in a personal use case), but we'd still need to figure out how we want to encode bytes in the ron format. Do you have a suggestion for what would be a good alternative?

Removing it would be a rather unpleasent breaking change for anyone using it however... Any thoughts?

That's my concern too. Anyone using ron with byte-encoded data would suddenly no longer be able to parse their config files. Since that would be a breaking change in the format and not in the crate, I think it would be difficult to justify outside of a major (major) version bump.

You can merge it as-is, this is just what came to my mind right now ^^

Thanks :)

@juntyr juntyr merged commit e2b0d97 into ron-rs:master Jan 10, 2023
@juntyr juntyr deleted the update-deps branch January 10, 2023 09:58
juntyr added a commit to juntyr/ron that referenced this pull request Aug 15, 2023
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