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

Allow global registration of custom XStream converters. #1010

Merged
merged 1 commit into from
Jun 18, 2017

Conversation

rankinc
Copy link
Contributor

@rankinc rankinc commented May 29, 2016

This lets us provide converters for any classes that we cannot annotate directly with @XStreamConverter. For example,

@XStreamConverters(
    @XStreamConverter(MyConverter.class),
    @XStreamConverter(MyOtherConverter.class)
)
@CucumberOptions(...)
@RunWith(Cucumber.class)
public class RunCukesTest {
}

The existing @XStreamConverter annotations are being reused for consistency. The RuntimeOptionsFactory will check for these annotations on the same class that has the @CucumberOptions annotation, which keeps this change nicely internal to cucumber-core.

This lets us provide converters for classes that we cannot annotate with @XStreamConverter directly.
@eduardmanas
Copy link

eduardmanas commented Jun 4, 2016

Nice one @rankinc!

This will enable using classes such as org.joda.time.DateTime without the need to annotate the method parameters with @Tranform all the time (which is a pain when the class is very common in your domain).

As an alternative, did you consider using @Transform instead of @XStreamConverters? For example, we could have re-used the @Transform annotation to annotate all Transformer implementations that should be automatically registered at start up.

@Transform
public class DateTimeTransformer extends Transformer<DateTime> {
    DateTimeFormatter dateTimeFormatter = DateTimeFormat.forPattern("yyyy-MM-dd HH:mm");
    @Override
    public DateTime transform(String value) {
        return dateTimeFormatter.parseDateTime(value);
    }
}

@rankinc
Copy link
Contributor Author

rankinc commented Jun 4, 2016

I did consider using @Transform instead of @XStreamConverter, yes. The problem is that Transformer extends SingleValueConverter, but my DataTable has more than one column.

However, if the principle of registering global converters in this way is acceptable then I don't see why we shouldn't register global transformers too.

TBH, I'm not a fan of using the @XStreamConverter annotation because its value is a ConverterMatcher rather than a Converter. I would prefer to use an annotation more like:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@Documented
public @interface CucumberConverter {

    Class<? extends Converter> value();

    int priority() default 0;

}

However, I figured this pull would be more likely to be merged if it reused existing annotations ;-).

@aslakhellesoy aslakhellesoy added Core ⚡ enhancement Request for new functionality labels Aug 30, 2016
@mpkorstanje mpkorstanje changed the title Allow registration of custom XStream converters. Allow global registration of custom XStream converters. Jun 18, 2017
@mpkorstanje mpkorstanje merged commit 9ec2d0f into cucumber:master Jun 18, 2017
mpkorstanje added a commit that referenced this pull request Jun 18, 2017
ConverterMatcher matcher = converter.value().newInstance();
if (matcher instanceof Converter) {
xStream.registerConverter((Converter) matcher, converter.priority());
}

Choose a reason for hiding this comment

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

I just came across this in attempting to use this feature (which is excellent!) It looks like SingleValueConverters are treated separately from other Converters and are not type-compatible, so in order to properly use SingleValueConverters with the global registration, there would need to be a second if statement here to handle them.

@lock
Copy link

lock bot commented Mar 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚡ enhancement Request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants