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

feat: validate int value range for IntField #1872

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

waketzheng
Copy link
Contributor

Description

Close #1853

Motivation and Context

Add a new validator for int fields to check value range when saving.

How Has This Been Tested?

make ci

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@waketzheng waketzheng mentioned this pull request Feb 5, 2025
7 tasks
Copy link

codspeed-hq bot commented Feb 5, 2025

CodSpeed Performance Report

Merging #1872 will degrade performances by 16.72%

Comparing waketzheng:validate-int-field (15150a2) with develop (1d4d60b)

Summary

❌ 1 regressions
✅ 11 untouched benchmarks

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

Benchmarks breakdown

Benchmark BASE HEAD Change
test_bulk_create_many_fields 10.7 ms 12.9 ms -16.72%

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13160816598

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 89.269%

Totals Coverage Status
Change from base Build 13160076672: 0.04%
Covered Lines: 6503
Relevant Lines: 7101

💛 - Coveralls

@waketzheng
Copy link
Contributor Author

@henadzit CodSpeed check failed. Int fields validator increases bulk_create by 2ms. Which one is better:

  • 1. Ignore it as 2ms is acceptable
  • 2. Refactor bulk_create to skip value validating for int fields

@henadzit
Copy link
Contributor

henadzit commented Feb 5, 2025

@waketzheng I think @markus-96 makes a good point about performance in his PR. I think it should be fine to have the int field constraints in to_db_value if it doesn't harm performance.

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.

IntField: constraints not taken into account
3 participants