Skip to content

Conversation

@fominok
Copy link
Contributor

@fominok fominok commented Apr 25, 2025

While this doesn't fixes #45 this PR is an implementation of an idea of removing lifetime parameter from Encoding trait as not really necessary there

edit: the branch is based on #46

@chipsenkbeil
Copy link
Owner

Similar to the other PR, we'd want to apply this to the utf8 encoding trait as well, right?

@fominok
Copy link
Contributor Author

fominok commented Apr 25, 2025

shall be better now, also bumped msrv badge

edit: I suppose crate version should be bumped as this is a breaking change

Comment on lines +11 to +12
[rustc_img]: https://img.shields.io/badge/rustc_1.65.0+-lightgray.svg
[rustc_lnk]: https://blog.rust-lang.org/2022/11/03/Rust-1.65.0/
Copy link
Owner

Choose a reason for hiding this comment

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

Is 1.65.0 the new minimum version that we'll need to support to do this? I was looking at bumping the minimum version, but hadn't done it yet, hence why we support a suuuuuuper old version of 1.58.1.

Reason I ask is to make sure I know what feature(s) from this new minimum version we need, and also we'll need to update both the Github action - some of the tests run with 1.58.1 - and the minimum version referenced in the Cargo.toml file.

I'm actually surprised that the code compiles and passes in Github actions because I thought we had some actions that are using rust 1.58.1 explicitly.

Anyway, two action items after scanning the contribution - thanks for doing this, btw! - are

  1. Update the minimum supported version in Cargo.toml
  2. Update the minimum test version used in .github/workflows/ci.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used cargo msrv find and it figured out that 1.65.0 is the version, which makes sense as there GATs were stabilized

Why it can pass CI on earlier versions I have no idea, unless those are nightly, where GATs could be for a while longer before

Copy link
Owner

Choose a reason for hiding this comment

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

@fominok I just merged a tiny PR that switches the github action being used to install the Rust toolchain. Reading the logs, seems like while it installed the minimum version it never switched to using it, hence not hitting errors.

So rebase your PR and then go in and update the minimum version from 1.58.1 to 1.65.0. This way, we can see it go from not compiling to compiling :)

@fominok fominok requested a review from chipsenkbeil April 25, 2025 15:11
@chipsenkbeil chipsenkbeil merged commit c02665b into chipsenkbeil:main Apr 25, 2025
17 checks passed
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.

with_platform_encoding doesn't compile

2 participants