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

🐛 Fix handling of UUID values having UUID.version=None #981

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin commented Sep 21, 2023

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 21, 2023

CodSpeed Performance Report

Merging #981 will degrade performances by 14.28%

Comparing uuid-cases (a9a4ce1) with main (4c84ed8)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 136 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main uuid-cases Change
test_dont_raise_error 39.2 µs 29.9 µs +31.2%
test_variable_tuple 34.3 µs 40 µs -14.28%

@lig
Copy link
Contributor

lig commented Sep 21, 2023

Fixed. Doesn't break Pydantic tests.

@lig lig marked this pull request as ready for review September 21, 2023 13:27
@lig
Copy link
Contributor

lig commented Sep 21, 2023

please review

@lig lig assigned davidhewitt and unassigned lig Sep 21, 2023
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #981 (a9a4ce1) into main (4c84ed8) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #981   +/-   ##
=======================================
  Coverage   93.09%   93.09%           
=======================================
  Files         106      106           
  Lines       15830    15830           
  Branches       26       26           
=======================================
  Hits        14737    14737           
  Misses       1086     1086           
  Partials        7        7           
Files Changed Coverage Δ
src/validators/uuid.rs 92.08% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c84ed8...a9a4ce1. Read the comment docs.

@@ -59,7 +59,7 @@ impl From<u8> for Version {
#[derive(Debug, Clone)]
pub struct UuidValidator {
strict: bool,
version: Option<Version>,
version: Option<usize>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change avoids the same conversion on every validation call.

@lig lig changed the title Fix UUID 🐛 Fix handling of UUID values having UUID.version=None Sep 21, 2023
Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Just one tiny suggestion

Comment on lines +97 to +100
if !match py_input_version {
Some(py_input_version) => py_input_version == expected_version,
None => false,
} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !match py_input_version {
Some(py_input_version) => py_input_version == expected_version,
None => false,
} {
if !matches!(py_input_version, Some(expected_version)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently, this doesn't work. It reports "unused variable: expected_version" and tests fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the suggestion is an equivalent to

                if !match py_input_version {
                    Some(expected_version) => true,
                    None => false,
                } {

which is not what we want here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes sorry, should have wanted:

!matches(py_input_version, Some(v) if v == expected_version)

or

!py_input_version.is_some_and(|v| v == expected_version)

@lig lig enabled auto-merge (squash) September 21, 2023 14:03
@lig lig merged commit 33a7cc0 into main Sep 21, 2023
29 checks passed
@lig lig deleted the uuid-cases branch September 21, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants