Don't format error messages on validators by nryoung · Pull Request #3407 · encode/django-rest-framework (original) (raw)

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

Already on GitHub?Sign in to your account

Conversation13 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

nryoung

In refs to: #3354

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

@tomchristie

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

  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

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

@nryoung

encode#3354

@nryoung

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

@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

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

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

@tomchristie

@xordoquy

@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

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

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

@tomchristie

Closing this off as stale.