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

improve quality of too short / too long error messages #990

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Sep 26, 2023

Change Summary

Reverts part of #902 as per pydantic/pydantic#7613 (comment)

Related issue number

Fixes pydantic/pydantic#7613

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @dmontagu

@davidhewitt davidhewitt changed the title don't force limit on generators when max_length is None don't force limit on generators when max_length is None Sep 26, 2023
@davidhewitt
Copy link
Contributor Author

please review

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #990 (25bb43c) into main (e610984) will decrease coverage by 0.01%.
The diff coverage is 97.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #990      +/-   ##
==========================================
- Coverage   92.99%   92.98%   -0.01%     
==========================================
  Files         106      106              
  Lines       15844    15827      -17     
  Branches       35       35              
==========================================
- Hits        14734    14717      -17     
  Misses       1103     1103              
  Partials        7        7              
Files Coverage Δ
src/errors/types.rs 99.66% <100.00%> (+<0.01%) ⬆️
src/input/return_enums.rs 84.93% <100.00%> (-0.46%) ⬇️
src/validators/generator.rs 90.94% <100.00%> (ø)
src/validators/list.rs 100.00% <ø> (ø)
src/validators/tuple.rs 94.32% <88.88%> (-0.03%) ⬇️

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 e610984...25bb43c. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 26, 2023

CodSpeed Performance Report

Merging #990 will degrade performances by 12.01%

Comparing dh/generators (25bb43c) with main (e610984)

Summary

❌ 1 regressions
✅ 137 untouched benchmarks

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

Benchmarks breakdown

Benchmark main dh/generators Change
test_variable_tuple 34.8 µs 39.5 µs -12.01%

@davidhewitt davidhewitt changed the title don't force limit on generators when max_length is None improve quality of too short / too long error messages Sep 26, 2023
@davidhewitt
Copy link
Contributor Author

Pushed a second commit to rework error messages as requested by @samuelcolvin.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM.

Do we want anyone else to check it? Otherwise i'm happy to merge.

@davidhewitt davidhewitt merged commit 8435153 into main Sep 26, 2023
28 of 30 checks passed
@davidhewitt davidhewitt deleted the dh/generators branch September 26, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generators raise ValidationError for list fields in v2.4
4 participants