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

BVAL-223 #41

Merged
merged 2 commits into from
Jan 28, 2013
Merged

BVAL-223 #41

merged 2 commits into from
Jan 28, 2013

Conversation

hferentschik
Copy link
Contributor

Creating pull request to an initial discussion and getting feedback on outstanding questions:

  • How to handle unknown properties and invalid expressions?
  • Should we explicitly reference a JSR? If so JSR 245 or JSR 341?
  • The need to define more functions than format and under which name it should be available.

following:<itemizedlist>
<listitem>
<para role="tck-testable">if the locale is passed explicitly to
the interpolator method via i<methodname>nterpolate(String,
Copy link
Contributor

Choose a reason for hiding this comment

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

"i" should be part of <methodname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@emmanuelbernard
Copy link
Contributor

Note after discussion with @hferentschik and me:

str:format or string:format - fairly neutral een though str seems more UI common and emmanuel prefers string a bit more

Clarify in the spec that string:format is bound to a Locale sensitive version of String.format (String#format(Locale, String, arg...))
We would say that we use the locale passed to the message interpolator (or use the default_

@gunnarmorling
Copy link
Contributor

Can I simply use String.format(...)

In the latest draft there is a syntax for directly referencing static members, but according to this mail this seems to be removed again. This mail speaks about "importing" types, but I can't find in the spec an example how this would be done.

@gunnarmorling
Copy link
Contributor

Clarify in the spec that string:format is bound to a Locale sensitive version of String.format (String#format(Locale, String, arg...))

Would one then always have to specify the local when invoking str:format()? Or do you suggest to bind two format() methods? AFAIK EL doesn't support overloaded functions bound to the same name, so we'd have to use different names in that case.

@hferentschik
Copy link
Contributor Author

In the latest draft there is a syntax for directly referencing static members, but according to this mail this seems to be removed again. This mail speaks about "importing" types, but I can't find in the spec an example how this would be done.

I tried to chase this down as well, but nothing. I think the FunctionMapper is the way to go. I also pinged Pete to get some feedback on the status of JSR 341...

@hferentschik
Copy link
Contributor Author

Would one then always have to specify the local when invoking str:format()? Or do you suggest to bind two format() methods? AFAIK EL doesn't support overloaded functions bound to the same name, so we'd have to use different names in that case.

I thought about two version, but you might be right that in this case we would need to function names. Not sure what's better in this case, two function names 'str:format' and 'str:formatWithLocale' vs a single format where I always have to pass the locale. The latter might not be so bad either.

@emmanuelbernard
Copy link
Contributor

On Fri 2013-01-18 10:51, Hardy Ferentschik wrote:

Would one then always have to specify the local when invoking str:format()? Or do you suggest to bind two format() methods? AFAIK EL doesn't support overloaded functions bound to the same name, so we'd have to use different names in that case.

I thought about two version, but you might be right that in this case we would need to function names. Not sure what's better in this case, two function names 'str:format' and 'str:formatWithLocale' vs a single format where I always have to pass the locale. The latter might not be so bad either.

Do you mean that one version would use the MessageInterpolator Locale
and the other version would access explicitly a Locale?

Do you guys see a use case for the second situation?

@hferentschik
Copy link
Contributor Author

Do you mean that one version would use the MessageInterpolator Locale and the other version would access explicitly a Locale?

What I mean is that we need a way to pass the Locale requested to the interpolate call to the EL. The only way to do this is to add the requested locale to the EL context as well and use the String#format version which requires an explicit locale, eg ${str:format(locale, "%1$2f", validatedValue}

@hferentschik
Copy link
Contributor Author

Right unfortunately, if I am right about my last comment, we are screwed. We need to make a choice:

break some apps and enforce that ${} has priority over {}: and deprecate {}
use your approach but then {} will live forever

Why can we not have the precedence of {} and deprecate it? For me the choices are

  • Never break backwards compatibility and hence support both cases ongoing
  • Break backwards compatibility
    • now, by giving ${} precedence
    • later, by still supporting {} and giving it precedence, but also deprecating it. Once (if we ever want to) we decide to
      not support it anymore (say BV 2.0), some patterns will behave differenty, most notable the ${value} example.

@emmanuelbernard
Copy link
Contributor

Do you mean that one version would use the MessageInterpolator Locale and the other version would access explicitly a Locale?

What I mean is that we need a way to pass the Locale requested to the interpolate call to the EL. The only way to do this is to add the requested locale to the EL context as well and use the String#format version which requires an explicit locale, eg ${str:format(locale, "%1$2f", validatedValue}

Can't you bind ${str:format( "%1$2f", validatedValue)} to String.format(interpolatedLocale, "%1$2f", validatedValue)?

That would be the best as the user does not have to deal with locales at all.

@emmanuelbernard
Copy link
Contributor

On Mon 2013-01-21 4:22, Hardy Ferentschik wrote:

Right unfortunately, if I am right about my last comment, we are screwed. We need to make a choice:

break some apps and enforce that ${} has priority over {}: and deprecate {}
use your approach but then {} will live forever

Why can we not have the precedence of {} and deprecate it? For me the choices are

  • Never break backwards compatibility and hence support both cases ongoing
  • Break backwards compatibility
    • now, by giving ${} precedence
    • later, by still supporting {} and giving it precedence, but also deprecating it. Once (if we ever want to) we decide to
      not support it anymore (say BV 2.0), some patterns will behave differenty, most notable the ${value} example.

You are correct but what I mean is that if we deprecate it, people
except to have a correct and non deprecated way of doing things. But it
turns out ${param} will return $paramValue in the deprecated world and
paramValue if we remove support for {} later.

So the only way to output paramValue is to use the deprecated solution.
Which I don't find satisfying.

The problem does not occur if we have ${} being processed before {}.

An alternative solution is to have the existing algorithm handle both {}
and ${} equally. But that is breaking the ${value} = $10 case, so we are
back to square 1.

Makes sense?

@hferentschik
Copy link
Contributor Author

Can't you bind ${str:format( "%1$2f", validatedValue)} to String.format(interpolatedLocale, "%1$2f", validatedValue)?

That would be the best as the user does not have to deal with locales at all.

Right, that would be the best, but afaiu that's not possible. See also http://docs.oracle.com/javaee/5/api/javax/el/FunctionMapper.html

@hferentschik
Copy link
Contributor Author

You are correct but what I mean is that if we deprecate it, people except to have a correct and non deprecated way of doing things. But it turns out ${param} will return $paramValue in the deprecated world and paramValue if we remove support for {} later. So the only way to output paramValue is to use the deprecated solution. Which I don't find satisfying. The problem does not occur if we have ${} being processed before {}. An alternative solution is to have the existing algorithm handle both {} and ${} equally. But that is breaking the ${value} = $10 case, so we are back to square 1. Makes sense?

Ok, get you. Maybe the best is to keep both, but I can send an email to the EG

@emmanuelbernard
Copy link
Contributor

On Mon 2013-01-21 6:45, Hardy Ferentschik wrote:

Can't you bind ${str:format( "%1$2f", validatedValue)} to String.format(interpolatedLocale, "%1$2f", validatedValue)?

That would be the best as the user does not have to deal with locales at all.

Right, that would be the best, but afaiu that's not possible. See also http://docs.oracle.com/javaee/5/api/javax/el/FunctionMapper.html

Bummer!

Can we cheat a bit and have our own
HibernateValidatorString.format(String, Object...) that would read the
locale somewhere? I can only think of a ThreadLocal validable but that
would make it easy for the user.

Try and propose that to the EG if you think that's a good idea.

</itemizedlist><phrase role="tck-testable">If the defined expression
is invalid or an unkown property is referenced the message expression
stays unchanged</phrase>.</para>

Copy link
Contributor

Choose a reason for hiding this comment

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

What is happening if an exception occurs during EL evaluation? Is the pattern then returned as is in any case? Or shall e.g. a ValidationException be thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no real distinction between parsing the expression and evaluating it. Generally I suggest to swallow the exception and return the pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, maybe it's worth to rephrase this sentence like this:

If an exception occurs during message interpolation, e.g. due invalid expressions or references to an unknown property, the message expression stays unchanged

?

@hferentschik
Copy link
Contributor Author

pushed one more version in this series ;-) Hopefully the last. Now using the Formatter wrapper approach. The spec just specifies that a bean needs to be available exposing format().


<listitem>
<para role="tck-testable">a bean mapped to the name
<constant>formatter</constant> exposing the the vararg method
Copy link
Contributor

Choose a reason for hiding this comment

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

Double "the"

@gunnarmorling
Copy link
Contributor

I made some minor spelling/wording remarks, but apart from that, the change looks great IMO now.

@hferentschik
Copy link
Contributor Author

Fixed the typos and removed the requirement for the 'null' literal in case of validatedValue being null.

@emmanuelbernard emmanuelbernard merged commit 3983938 into jakartaee:master Jan 28, 2013
@hferentschik hferentschik deleted the BVAL-223 branch January 28, 2013 14:35
@hferentschik
Copy link
Contributor Author

Thanks

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.

3 participants