Skip to content

Don't format error messages on validators #3407

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

Closed
wants to merge 1 commit into from

Conversation

nryoung
Copy link
Contributor

@nryoung nryoung commented Sep 16, 2015

In refs to: #3354

This removes .format calls on error messages and defaults to using the default class attribute error message.

@tomchristie
Copy link
Member

Couple of things here:

  1. This doesn't appear to remove it everywhere, eg CharField?
  2. I think we'd still want to pass the message through. (So they can be specified at the field class level)

@nryoung
Copy link
Contributor Author

nryoung commented Sep 18, 2015

  1. This doesn't appear to remove it everywhere, eg CharField?

Good catch, however, when I remove this from the CharField it seems that self.message is not assigned? I am not entirely sure what ungettext_lazy is doing on the MaxLengthValidator: https://github.com/django/django/blob/stable/1.7.x/django/core/validators.py#L284-L287

  1. I think we'd still want to pass the message through. (So they can be specified at the field class level)

Do you mean something along the lines of?:

self.validators.append(MaxLengthValidator(self.max_length, message=self.message))

@tomchristie
Copy link
Member

Do you mean something along the lines of

That's what I was thinking - tho let's look to Django's Field classes for direction.

@nryoung
Copy link
Contributor Author

nryoung commented Sep 19, 2015

let's look to Django's Field classes for direction.

Looking at both Django's IntergerField and CharField, only the value is passed in to the validator: https://github.com/django/django/blob/master/django/db/models/fields/__init__.py#L1881, https://github.com/django/django/blob/master/django/db/models/fields/__init__.py#L1094

 encode#3354

- Update tests to reflect new error messages provided by Django field
  parent classes
@nryoung
Copy link
Contributor Author

nryoung commented Sep 20, 2015

OK @tomchristie, I followed Django's Field classes here and made changes accordingly. message is not passed in to the validator on initialization.

nedbat pushed a commit to openedx/edx-platform that referenced this pull request Sep 22, 2015
TNL-3373

Django REST Framework has a pull request to fix this problem:
encode/django-rest-framework#3407

We want it now, though, so I picked it onto their tip in my own fork.
Once they merge it and release, we can put this back to an official
release.
@xordoquy xordoquy added this to the 3.2.5 Release milestone Sep 23, 2015
@xordoquy
Copy link
Collaborator

@nryoung I'm not sure your last commit what was we had in mind.
It breaks the current error message customization scheme. Error messages for min/max will now be within the MaxLengthValidator/MinLengthValidator instead of CharField.
This might break anyone that tuned its error message.

Can't we instead simply pass the unformatted message to the Validator and be using the Validation's formatters instead of the current format ?

@xordoquy
Copy link
Collaborator

ie:

    default_error_messages = {
        ...
        'max_length': _('Ensure this field has no more than {max_length} characters.'),
        ...
    }

would become:

    default_error_messages = {
        ...
        'max_length': ungettext_lazy(
            'Ensure this value has at most %(limit_value)d character (it has %(show_value)d).',
            'Ensure this value has at most %(limit_value)d characters (it has %(show_value)d).',
            'limit_value')
        ...
    }

@nryoung
Copy link
Contributor Author

nryoung commented Sep 23, 2015

@xordoquy understood. I misunderstood what @tomchristie was asking for. I can probably get this fixed up tomorrow.

@tomchristie
Copy link
Member

@xordoquy @nryoung That's a separate issue, no?

@xordoquy
Copy link
Collaborator

@tomchristie I don't think so. In my snippet it should defer the format to the moment the validation is performed which should fix the initial issue.

@tomchristie
Copy link
Member

Okay, so how would you rephrase it without bringing in the (unrelated) pluralization issue.

The problem is that the lazy translation is being evaluated at the point that we format it, no?

@xordoquy
Copy link
Collaborator

@tomchristie it's a bit more complex than it appears. Will open a new PR a bit later.

@tomchristie
Copy link
Member

Closing this off as stale.

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

Successfully merging this pull request may close these issues.

3 participants