rust-bakery / nom

Rust parser combinator framework
MIT License
9.18k stars 792 forks source link

Bump MSRV to 1.57.0 to more correctly match the codebase. #1760

Closed martinfrances107 closed 1 month ago

martinfrances107 commented 1 month ago

As the code has been updated new features that break the currently stated MSRV have been added.

I just want the codebase to reflect reality.

This issue can be seen by running cargo clippy

warning: current MSRV (Minimum Supported Rust Version) is `1.56.0` but this item is stable since `1.57.0`
   --> src/traits.rs:649:10
    |
649 |     self.as_slice()
    |          ^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
    = note: `#[warn(clippy::incompatible_msrv)]` on by default
codspeed-hq[bot] commented 1 month ago

CodSpeed Performance Report

Merging #1760 will degrade performances by 14.2%

Comparing martinfrances107:bump_MSRV (367c529) with main (2560d5c)

Summary

⚡ 1 improvements ❌ 1 regressions ✅ 22 untouched benchmarks

:warning: _Please fix the performance issues or acknowledge them on CodSpeed._

Benchmarks breakdown

Benchmark main martinfrances107:bump_MSRV Change
recognize float str 1 µs 1.2 µs -14.2%
number 287.2 ns 259.4 ns +10.71%
codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 61.42%. Comparing base (2560d5c) to head (367c529). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1760 +/- ## ========================================== - Coverage 61.47% 61.42% -0.05% ========================================== Files 24 23 -1 Lines 2951 2950 -1 ========================================== - Hits 1814 1812 -2 - Misses 1137 1138 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

martinfrances107 commented 1 month ago

looking at the 14% drop in preformance ( A change to 1 µs to 1.2 µs ) well that looks completely to be expect from from repeatibility noise.

The +10% imrpovement ( from 287.2 ns to 259.4 ns ) is a welcome minor improvement

In short, I think being blocked on this is misguided.

Geal commented 1 month ago

chill a bit. Benchmarks can be flaky, that's not an issue, and this change definitely cannot affect performance