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

Update validators.py #18284

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

Conversation

majid-vaghari
Copy link
Contributor

Fix unnecessary lazy evaluation in BaseValidator

Trac ticket number

ticket-N/A

Branch description

if you run this:

from django.utils.translation import gettext_lazy
from django.core.validators import MinValidator

MinValidator(0, message=gettext_lazy('hi'))

This will raise:

django.core.exceptions.AppRegistryNotReady: The translation infrastructure cannot be initialized before the apps registry is ready. Check that you don't make non-lazy gettext calls at import time.

And the problem is this line because it tries to evaluate message instead of checking if it is None. This PR will solve this issue

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Fix unnecessary lazy evaluation in BaseValidator
@majid-vaghari
Copy link
Contributor Author

Alternatively the __bool__ method on lazy text object could be overridden but this seemed easier. 🤷🏻‍♂️

@jaap3
Copy link
Contributor

jaap3 commented Jun 21, 2024

This would be a change in behaviour, for example:

MinValidator(0, message="")

Will now override the message, while before it wouldn't.

It appears there's nothing depending on that behaviour in Django's test suite, and it seems like reasonable behaviour, but someone might be depending on the old behaviour.

Adding tests will help clear up the behaviour and prevent future regressions.

@majid-vaghari
Copy link
Contributor Author

This would be a change in behaviour, for example:

MinValidator(0, message="")

Will now override the message, while before it wouldn't.

It appears there's nothing depending on that behaviour in Django's test suite, and it seems like reasonable behaviour, but someone might be depending on the old behaviour.

Adding tests will help clear up the behaviour and prevent future regressions.

I see your point but if you examine the file mentioned, all other validator classes use message is not None when they want to check the message. Also, this call to evaluate lazy text objects is not necessary. So it feels like this is the intended behavior. Nevertheless I also agree that adding tests for these behaviors is beneficial and more verbose. I'll try to add them in the following day

@majid-vaghari
Copy link
Contributor Author

@jaap3 I tried adding test cases in tests/validators/tests.py But it seems in here django is already setup. For example if I add this test:

class TestGetTextLazy(SimpleTestCase):
    def test_min_value(self):
        print('hi')
        print(_('hello'))

It runs with no problems whatsoever. Can you help me where I can test django so it is not already setup and apps are not loaded?

@majid-vaghari
Copy link
Contributor Author

Can anyone please check this pull request?

@majid-vaghari
Copy link
Contributor Author

@jaap3 Can you please check this?

@jaap3
Copy link
Contributor

jaap3 commented Jul 15, 2024

Sorry, I don't know how to get a testcase working for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants