Getting knowledge about the current Locale within any custom @DefaultParameterTransformer

[ghostcity]

🤔 What's the problem you're trying to solve?

Hi, I've seen that the BuiltInParameterTransformer is instanziated during the initial ramp-up:

io.cucumber.core.runtime.Runtime#executePickle(Pickle)
calls io.cucumber.core.runner.Runner#runPickle(Pickle)
calls io.cucumber.core.runner.CachingGlue#prepareGlue(Locale)
instanciates io.cucumber.core.stepexpression.StepTypeRegistry#StepTypeRegistry(Locale)
instanciates io.cucumber.cucumberexpressions.ParameterTypeRegistry#ParameterTypeRegistry(Locale)
instanciates io.cucumber.cucumberexpressions.BuiltInParameterTransformer#BuiltInParameterTransformer(Locale)

Doing so, the BuiltInParameterTransformer is aware of the language declaration of the current Feature file, and thus can parse numbers accordingly to the English/French/German/... number delimiters etc.

In opposite, any method annotated with @DefaultParameterTransformer does not know about this current Locale. The locale information is neither injected into the transform method nor is it injected into the constructor of the class declaring the @DefaultParameterTransformer-annotated method.

In result, any custom ParameterTransformer has no chance to do a number transform (or whatever Locale aware transform) similar to what the BuiltInParameterTransformeris able to do. Actually, I do not see a chance to bring the Locale information into the custom transformation. However, I would like to add such feature -- I just don't know, what would be the preferred way.

✨ What's your proposed solution?

Can you give me a hint, where I should cache the current Feature's locale information and where this Locale should be injected into the transform process of the custom ParameterTransformer?

Giving me such a hint would help me to create an MR. (That is my proposed solution: Id like to implement that missing code and create a MR.)

Or in general, what do you think about my feature request. Is this a proper idea?

⛏ Have you considered any alternatives or workarounds?

Yes, but I do not see any proper way to gain access to the Locale that has been found within the current Pickle processed by the Cucumber-runner.

📚 Any additional context?

No response

[mpkorstanje]

Giving me such a hint would help me to create an MR. (That is my proposed solution: Id like to implement that missing code and create a MR.)

Probably the best way to do this would be to store the locale in the ParameterTypeRegistry.

The ParameterByTypeTransformer would have to be made locale aware. And I think adding a LocaleParameterByTypeTransformer would be the easiest way to do that. You can then change the type of the ParameterTypeRegistry.defaultParameterTransformer to the locale aware version.

To avoid breaking changes, please do add an overloaded version of setDefaultParameterTransformer and wrap the arguments for existing setDefaultParameterTransformer so that they also become locale aware (only to ignore it).

For consistency, I think it would also make sense to make KeyboardFriendlyDecimalFormatSymbols public.

Is that enough to get you started?

[ghostcity]

Yeah, absolut. Exactly the answer I was hoping for, because I'd like to do it the way it fits best into the existing concept.
Let's see, if I understood it correctly ...

[ghostcity]

Hi, I tried the proposed solution but at the end this new code will never come into action.

This is because Cucumber users never declare its own custom ParameterByTypeTransformer. Instead, they "only" declare a transform-method and attach the annotation @DefaultParameterTransformer. This method will be detected within GlueAdaptor#addDefinition(Method, Annotation) via:

if (annotationType.equals(DefaultParameterTransformer.class)) {
    glue.addDefaultParameterTransformer(new JavaDefaultParameterTransformerDefinition(method, lookup));
}

So whatever custom transformer class a user is implementing, at the end the GlueAdaptor will use a fresh instance of JavaDefaultParameterTransformerDefinition. Currently, this JavaDefaultParameterTransformerDefinition is not an extension of the new LocaleParameterByTypeTransformer.

So, what do you think? Shall I extend the JavaDefaultParameterTransformerDefinition to make it a LocaleParameterByTypeTransformer? Or shall I add a new Annotation, e.g. @DefaultLocalParameterTransformer causing a fresh instance of a JavaDefaultLocaleParameterTransformerDefinition? (This new class will be a sub-class of DefaultLocalParameterTransformer.)

The first solution would change the existing code (without breaking it, it will be compatible with previous cucumber versions), the second solution would not change but extend the existing code but could lead into a missunderstanding (because a user could assume he must provide both, a @DefaultParameterTransformer and a @DefaultLocalParameterTransformer).

[mpkorstanje]

I think it should be possible to support multiple method signatures for the JavaDefaultParameterTransformerDefinition.

The current:

public Object defaultDataTableEntry(String fromValue, Type toValueType)

And then the new:

public Object defaultDataTableEntry(String fromValue, Type toValueType, Locale locale)

Then adding a new DefaultParameterTransformerDefinition.localeParameterByTypeTransformer() that defaults to calling the parameterByTypeTransformer() if not implemented should suffice.

[ghostcity]

Yepp, adding such a method looks promising. I'll try it out.

[antagoony]

I tried to implement the code following your proposals. At the end it turned out that the ParameterTypeRegistry does not need to store the current Locale (your first response), because the method DefaultParameterTransformerDefinition.localeParameterByTypeTransformer() (your second response) needs to provide the Locale itself already. And even further, the changes within "cucumber-jvm" do not necessarily require a LocaleParameterByTypeTransformer. The Locale-awareness can be implemented without such extended interface. However, providing such an explicit interface would make things more clear.

Independently, I think the KeyboardFriendlyDecimalFormatSymbols should become public in any case because users probably would like to adopt this existing feature when they start to provide their own custom Locale-aware type transformation. (At least I will do so ;-)

So, this is my proposal:

• make the KeyboardFriendlyDecimalFormatSymbols a public class
  • see PR "Make KeyboardFriendlyDecimalFormatSymbols a Public Class"
• optionally inject the current Locale within the custom @DefaultParameterTransformer method
  • see PR "Locale-aware DefaultParameterTransformer"

In addition, your propose to declare a LocaleParameterByTypeTransformer will make things within "cucumber-expressions" more obvious. I did not create a PR yet, instead I think the required change can be used to ask for feedback if this is going into the right direction: Draft-PR "Preview of explicit LocaleParameterByTypeTransformer within cucumber-expressions".

In case this new LocaleParameterByTypeTransformer will be included within a future release of "cucumber-expressions", it afterwards can be adapted by the "cucumber-jvm". Doing so, the code could look like this: Draft-PR "Preview of Adaptation of explicit LocaleParameterByTypeTransformer within cucumber-jvm". Again, this is my current draft and I'd like to know if this matches what you had in mind when responding me.

Additonal note: Within the PR "Locale-aware DefaultParameterTransformer" I did not name the new method localeParameterByTypeTransformer (your propose by your second response) because the return type is still a ParameterByTypeTransformer. Within the Draft-PR "Preview of Adaptation of explicit LocaleParameterByTypeTransformer within cucumber-jvm" the return type is changed to the new LocaleParameterByTypeTransformer and thus the method then is renamed accordingly.

[mpkorstanje]

Sounds like you found a short cut. 😄

To ensure there is a single source of truth for transforms I think it would be better if the locale is stored in the ParameterTypeRegistry. if not the locale would also sit in every closure returned by localeParameterByTypeTransformer.

Thoughts?

[antagoony]

The challenge is to get the locale information to every code position where the current locale-unaware #transform(String, Type) is called. Beside the ParameterTypeRegistry itself this is CucumberExpression and RegularExpression.

What about this? Storing the Locale instance within the ParameterTypeRegistry (your propose) and, in addition, providing a package private getter to that locale information? In result both, the CucumberExpression and the RegularExpression, can refer to this locale getter and reuse it when calling the new locale-aware tranform method. (Both types already hold a reference to ParameterTypeRegistry.)

As a further propose, this additional commit extends the aforementioned Draft-PR "Preview of explicit LocaleParameterByTypeTransformer within cucumber-expressions".

If this will be the new code of "cucumber-ecpressions", then there will be no further need to store the Locale within the closure instance of ParameterByTypeTransformer indeed.

So what do you think? Is this the preferred solution for "cucumber-expression"? If yes, I would propose a new PR of "cucucmber-jvm" adapting this new situation.

[mpkorstanje]

What about this? Storing the Locale instance within the ParameterTypeRegistry (your propose) and, in addition, providing a package private getter to that locale information? In result both, the CucumberExpression and the RegularExpression, can refer to this locale getter and reuse it when calling the new locale-aware tranform method.

Sounds reasonable. Please propose the PR!