Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: enforce Decimal type in min_value and max_value arguments of DecimalField#8972

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
auvipy merged 2 commits intoencode:masterfromIsmaelJS:master
May 9, 2023

Conversation

@IsmaelJS
Copy link
Contributor

Description

Fixes issue#8963

@auvipyauvipy self-requested a reviewMay 4, 2023 12:45
Copy link
Collaborator

@auvipyauvipy left a comment

Choose a reason for hiding this comment

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

I am wondering if this enforcement create some noise or might break existing codes which were not using float and decimal in right separation? in that case should we also improve the docs if / where necessary

@IsmaelJS
Copy link
ContributorAuthor

I am wondering if this enforcement create some noise or might break existing codes which were not using float and decimal in right separation? in that case should we also improve the docs if / where necessary

I think the change may "break" the code but actually is fixing bugs on projects in which those input arguments are not decimal types.
Another approach is to warn a message instead of raising a ValueError, but this doesn't fix/prevent the error.
I agree to document the input type required for these arguments in DecimalField field.

auvipy reacted with thumbs up emoji

@IsmaelJSIsmaelJS requested a review fromauvipyMay 4, 2023 16:17
@auvipyauvipy added this to the3.15 milestoneMay 4, 2023
@kevin-brown
Copy link
Contributor

My preference would be to trigger a warning for now and put this through the deprecation cycle. We previously allowed floats, ints, and Decimals and now we are only allowing Decimal. I definitely agree with the goal, but breaking it immediately is probably not the right move.

auvipy and vadym-osetskyy reacted with thumbs up emoji

@auvipy
Copy link
Collaborator

My preference would be to trigger a warning for now and put this through the deprecation cycle. We previously allowed floats, ints, and Decimals and now we are only allowing Decimal. I definitely agree with the goal, but breaking it immediately is probably not the right move.

I think we should keep supporting Int, float & decimal all. and we should mostly align with django first

@IsmaelJS
Copy link
ContributorAuthor

My preference would be to trigger a warning for now and put this through the deprecation cycle. We previously allowed floats, ints, and Decimals and now we are only allowing Decimal. I definitely agree with the goal, but breaking it immediately is probably not the right move.

I agree with that a first step should be warn the situation instead of break. The point of@auvipy about aligning with Django is also interesting, because actually a [Min|Max]ValueValidator with any number type can be added to a models.DecimalField. Thus, do you agree guys to only leave a warning message in this PR?

@auvipy
Copy link
Collaborator

oh I see I miss read it. decimalField should support decimals only. but as it might break existing codes, we should only start with the warning for now :)

@auvipy
Copy link
Collaborator

we should move towards the path of django but also provide the users a graceful way to fix their code

@IsmaelJS
Copy link
ContributorAuthor

IsmaelJS commentedMay 7, 2023
edited
Loading

oh I see I miss read it. decimalField should support decimals only. but as it might break existing codes, we should only start with the warning for now :)

@auvipy done! I also included a test.

@auvipyauvipy merged commit99e8b40 intoencode:masterMay 9, 2023
@terencehonles
Copy link
Contributor

It looks like this finally got released, so sorry for the delay, but this really should have used warnings instead of logging a warning. We run our tests flagging warnings as errors, but I just happened to see this in the logs and it was less obvious than it could have been to track down where this was coming from.

I assume this is really only a problem when usingfloat for a min/max value, and we're using anint where this warning could have been ignored.

viniciusd reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@auvipyauvipyauvipy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.15

Development

Successfully merging this pull request may close these issues.

4 participants

@IsmaelJS@kevin-brown@auvipy@terencehonles

[8]ページ先頭

©2009-2025 Movatter.jp