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

Cucumber expressions: Remove single transformer #357

Merged
merged 4 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@aslakhellesoy
Contributor

aslakhellesoy commented Apr 9, 2018

I didn't like the choices between Transformer and Function, so I removed Function (and SingleTransformer).

Merging this will cause cucumber/cucumber#1248 to break, but since it's never been released I'm not sure we need to bump the minor release.

I will have to make some minor consistency changes to Ruby and JavaScript, but I don't think it will affect behaviour externally.

@aslakhellesoy aslakhellesoy changed the base branch from master to cr-goLang Apr 9, 2018

@aslakhellesoy aslakhellesoy changed the base branch from cr-goLang to master Apr 9, 2018

@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 10, 2018

Ideally, when using a lambda expression, you won't notice those. By defining the lambda as (String[] matches) -> .... or (String match) -> .... you can decide if you want to use all parameter or only the first one. I expect that most parameter types will use the whole matching making this a nice bit of sugar.

The reason we have a function class is because I had to back port this to 1.7 which doesn't support lambdas yet (but this won't prevent clients at 1.8 from using them).

In javascript you can get the same behaviour when using transformFn.call(null, matches). If the transformFn is defined as function(arg1, arg2, ... argN) the individual strings in the matches array will be bound to the arguments.

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 10, 2018

I've done a comparison of before and after, on Java 7 and 8. I like the more concise API - not having to choose between Function and Transformer (which could be confusing for users).

I don't like having to use String[] or String... in the signatures though, and it seems we lost the ability to use method references for (String) constructors (which are more common).

Is there a way we can have both a less confusing API and the nice syntax sugar?

Java 7

Before PR:

new Function<String, Color>() {
    @Override
    public Color apply(String s) {
        return new Color(s);
    }
}

After PR:

new Transformer<Color>() {
    @Override
    public Color transform(String... args) {
        return new Color(args[0]);
    }
}

Java 8

Before PR:

Color::new

After PR:

args -> new Color(args[0])
@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 10, 2018

Ok @mpkorstanje I added some more generics, bringing back support for single argument transforms while keeping a single interface.

I couldn't figure out a way to avoid the static factory methods ParameterType.single and ParameterType.multi, but perhaps that's not so bad?

WDYT?

@aslakhellesoy aslakhellesoy referenced this pull request Apr 10, 2018

Merged

[Core] Implement cucumber expressions #1248

4 of 4 tasks complete
@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 10, 2018

You've found the only two ways to do this. Java needs the type information which requires that you spell out exactly what each method looks like. And as you've also noticed generics are erased which means you also need to use static factory methods to differentiate when using different generic types.

So either the information about the number of arguments is contained in the lambda signature (String[] args) -> ... vs (String arg) -> ... or it is contained in the name of the static factory method ParameterType.single vs ParameterType.multi.

Given that choice, the type information has to be conveyed one way or the other, I would prefer to use the lambda signature as it conveys more clearly what the type of args will be and does so very near the location that args will be used. The static factory method conveys it from further away and does so implicitly.

It is also important to keep in mind is that the DataTableType in datatable follows the same pattern. If we change to static constructors here we should do so too for DataTableType. And looking at the difference between DataTableType and ParameterType I can see that that the former spells out the different transformers TableTransformer, TableEntryTransformer, TableRowTransformer and TableCellTransformer. In the latter we have Transformer and Function. So I think we just need to come up with a better name for those two.

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 10, 2018

Ok. How about Transformer (using String) and MultiTransformer (using String[])?

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 10, 2018

I added MultiTransformer and got rid of the static ctors.

The only thing that annoys me now is that method references only work if I cast:

(Transformer<Currency>) Currency::getInstance

Should we try to find a way to avoid that?

@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 10, 2018

Should we try to find a way to avoid that?

You can't really avoid that. Both ParameterType and Currency::getInstance are overloaded. This combination prevents resolution. It will work fine with non overloaded method references.

@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 10, 2018

I've removed Function all together. We only expose Transformer<T> and MultiTransformer<T> so ParameterType.transformer will always be either one or the other. We could try to capture this somehow but this provides us no benefit.

So I've put the adapter pattern back in again. But it is no longer exposed to the world.

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 11, 2018

@mpkorstanje I tried it out with Java8, and this doesn't seem to work with method references. Did you try it?

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 11, 2018

It does work if I refactor the ParameterType constructors to swap Class<T> type and MultiTransformer<T> transformer. It removes the ambiguity.

Seems like an acceptable workaround to me.

@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 11, 2018

Yes! They work for me provided you're using one that is not overloaded. This means Currency::getInstance just won't won't work in this setup. I don't think this is such a bad thing though.

I do think switching parameter order is worse. It makes the API inconsistent.

If you think this is a must we could make the Transform a concrete class with only public static constructors for both use cases:

<T> Transform.transform(Function<String,T>)
<T> Transform.transformCaptureGroups(Function<String[], T>)
@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 11, 2018

I think it would be a shame not to be able to use method references. User code would be very verbose without them. Not sure how you'd implement it though - want to give it a go?

@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 11, 2018

I can show you trade offs. The information has to come from somewhere.

@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 11, 2018

@aslakhellesoy check this:

// The base case. Create a color from a string.
new ParameterType<>(
"color",
"r|g|b",
Color.class,
Color::getInstance,
true,
true
);
// The other base case. Create a chroma from an array of strings.
new ParameterType<>(
"chroma",
"r|g|b",
Chroma.class,
Chroma::getInstance,
true,
true
);
// The static helper method.
// Slightly more verbose because we're using a static constructor to
// create the Transform. Also note that the the generic type of ParameterType can't be inferred now.
// (This also means we can get rid of it entirely if we move up to Java 8 and if we chose to go with the base case)
new ParameterType<Color>(
"color",
"r|g|b",
Color.class,
transform(Color::getInstance),
true,
true
);
new ParameterType<Chroma>(
"chroma",
"r|g|b",
Chroma.class,
transformCaptureGroups(Chroma::getInstance),
true,
true
);
// So how do we handle over loading?
// The base case with over loading:
// It's not very verbose. The lambda is even shorter.
new ParameterType<>(
"color",
"r|g|b",
OverloadedColor.class,
(String arg) -> OverloadedColor.getInstance(arg),
true,
true
);
new ParameterType<>(
"color",
"r|g|b",
OverloadedColor.class,
(Transformer<OverloadedColor>) OverloadedColor::getInstance,
true,
true
);
// The static helper method case with overloading:
// This is indeed a bit less verbose but comes at the cost of **always** having to use the static helper.
// And it doesn't even safe that much! Like 20 characters or so.
new ParameterType<OverloadedColor>(
"color",
"r|g|b",
Color.class,
transform(OverloadedColor::getInstance),
true,
true
);
// It's even more obvious when using the longer transformCaptureGroups name
// It's actually longer then the lambda (and again note the requirement for the generic in ParameterType.
// Arguably you can avoid this by using a shorter name but the gains are minimal.
// As such I think it's okay to use lambda's when static constructors are overloaded.
new ParameterType<OverloadedChroma>(
"chroma",
"r|g|b",
OverloadedChroma.class,
transformCaptureGroups(OverloadedChroma::getInstance),
true,
true
);
new ParameterType<>(
"chroma",
"r|g|b",
OverloadedChroma.class,
(String[] args) -> OverloadedChroma.getInstance(args),
true,
true
);
new ParameterType<>(
"chroma",
"r|g|b",
OverloadedChroma.class,
(MultiTransformer<OverloadedChroma>) OverloadedChroma::getInstance,
true,
true
);
}

I faked out a few parts to just get the point across.

Broadly considered we have to make a trade off between 3 options.

  1. We only support a single transform (String[] args) -> ..... This means that people will often have to do args[0] in their lambda's and can only use static constructors that accept an string array. Otherwise it's very concise.

  2. We support (String[] args) -> .... and (String arg) -> ..... This means that people can use the more common static constructors that accept a string. They won't be able to use overloaded method references but is otherwise quite concise. The verbosity of a lambda is not much higher then a method reference.

  3. We support transformCaptureGroups((String[] args) -> ....) and transform(String arg) -> ....). This means people can use overloaded method references but adds a constant cost to all cases and doesn't improve the verbosity much when dealing with over loaded method references.

I reckon option 1 is right out. Normally I'd data mine the choice between one and 2 and 3, but from experience I'd say 2 is the best one.

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Apr 11, 2018

I went for number 2 and merged your changes to master @mpkorstanje. Constructor method references are working nicely on Java 8. I'm not sure what's changed since they didn't work yesterday, but all seems good now.

Let me know if you're happy for me to make a new cucumber-expressions release.

@mpkorstanje mpkorstanje reopened this Apr 11, 2018

@mpkorstanje mpkorstanje merged commit c77808c into master Apr 11, 2018

1 check was pending

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@mpkorstanje

This comment has been minimized.

Contributor

mpkorstanje commented Apr 11, 2018

I've renamed the MultiTransformer to CaptureGroupTransformer. It doesn't transform capture groups, but it does transform the parts of the string matching those specific capture group. So... close enough.

You didn't revert the commit in which you swapped the parameter order. I've fixed that too.

I'm happy for now. Lets release this!

@mpkorstanje mpkorstanje deleted the cucumber-expressions-remove-single-transformer branch Apr 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment