Skip to content

implementing validate_strings #883

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

Merged
merged 2 commits into from
Sep 19, 2023
Merged

implementing validate_strings #883

merged 2 commits into from
Sep 19, 2023

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin commented Aug 14, 2023

  • Complex: do something clever with InputValue and/or as_error_value to either make error.into_owned() cheaper or reduce its use dh - resolved in make error "duplicate" cheaper #950
  • Mechanical: needs to work on dataclass as well as model and typed_dict
  • Trivial: Move the String implementation of Input back into input_json as it's ownly used by JSON keys.
  • (from @davidhewitt) - rename to validate_strings

Selected Reviewer: @dmontagu

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 14, 2023

CodSpeed Performance Report

Merging #883 will improve performances by 34.14%

Comparing validate_string (126bebc) with main (1d10238)

Summary

⚡ 3 improvements
✅ 135 untouched benchmarks

Benchmarks breakdown

Benchmark main validate_string Change
test_chain_function 46.8 µs 34.9 µs +34.14%
test_chain_nested_functions 56.3 µs 43.5 µs +29.56%
test_variable_tuple 39.4 µs 34.2 µs +15.2%

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #883 (72f979a) into main (245381f) will decrease coverage by 0.63%.
Report is 5 commits behind head on main.
The diff coverage is 60.81%.

❗ Current head 72f979a differs from pull request most recent head 126bebc. Consider uploading reports for the commit 126bebc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
- Coverage   93.78%   93.16%   -0.63%     
==========================================
  Files         105      106       +1     
  Lines       15539    15744     +205     
  Branches       25       25              
==========================================
+ Hits        14574    14668      +94     
- Misses        959     1070     +111     
  Partials        6        6              
Files Changed Coverage Δ
src/errors/mod.rs 92.30% <ø> (ø)
src/input/mod.rs 100.00% <ø> (ø)
src/validators/typed_dict.rs 96.49% <0.00%> (-0.86%) ⬇️
src/input/input_string.rs 29.92% <29.92%> (ø)
src/input/input_abstract.rs 86.76% <45.45%> (-3.64%) ⬇️
src/validators/union.rs 89.06% <50.00%> (-0.24%) ⬇️
src/validators/dict.rs 87.30% <80.00%> (-0.67%) ⬇️
src/validators/generator.rs 90.94% <85.71%> (ø)
src/input/return_enums.rs 85.30% <86.66%> (+0.03%) ⬆️
src/input/input_json.rs 90.47% <88.88%> (-1.36%) ⬇️
... and 12 more

... and 1 file with indirect coverage changes


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 245381f...126bebc. Read the comment docs.

@samuelcolvin samuelcolvin force-pushed the validate_string branch 2 times, most recently from 02106d5 to a9828c8 Compare August 23, 2023 15:31
@samuelcolvin samuelcolvin changed the title implementing validate_string implementing validate_string Sep 5, 2023
@davidhewitt davidhewitt mentioned this pull request Sep 7, 2023
4 tasks
@davidhewitt davidhewitt force-pushed the validate_string branch 3 times, most recently from 730d631 to b69be8f Compare September 18, 2023 12:09
@davidhewitt davidhewitt changed the title implementing validate_string implementing validate_strings Sep 18, 2023
@davidhewitt davidhewitt marked this pull request as ready for review September 19, 2023 15:26
@davidhewitt
Copy link
Contributor

please review

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.

3 participants