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.

Sign up for GitHub

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

Jump to bottom

Add argument conversions for Currency, Locale, and URI #1221

Merged
merged 1 commit into from
Dec 30, 2017
Merged

Add argument conversions for Currency, Locale, and URI #1221

merged 1 commit into from
Dec 30, 2017

Conversation

hisener
Copy link
Contributor

@hisener hisener commented Dec 30, 2017 •
edited
Loading

Overview

Add argument conversions for Currency, Locale, and URI

Related issue: #1218

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

hisener
hisener commented Dec 30, 2017
View reviewed changes
...upiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java
@@ -184,4 +182,26 @@ public Object convert(String source, Class<?> targetType) throws Exception {
}
}

static class StringToJavaMiscConverter implements StringToObjectConverter {
Copy link
Contributor Author

@hisener hisener Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Duplicate code with StringToPrimitiveConverter class. Maybe we can merge with StringToPrimitiveConverter class by changing its name?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't quite follow you.

Can you please expound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbrannen If you look at StringToPrimitiveConverter and this class. They are very similar. Maybe, we can use a single class instead of two. I am not sure what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hisener I think the same argument could be made for StringToJavaTimeConverter. I personally think it's fine to have StringToJavaMiscConverter be separate from StringTo(Primitive|JavaTime)Converter. They could all share an abstract class if it would save lines of code. WDYT @sbrannen?

Copy link
Member

Choose a reason for hiding this comment

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

I agree: let's keep them separate.

I don't think the minor code duplication is an issue here (yet).

@hisener hisener force-pushed the master branch 3 times, most recently from 699ba4d to 96ee36a Compare December 30, 2017 11:51
@marcphilipp
Copy link
Member

Thanks for the PR! Please also update the Javadoc of DefaultArgumentConverter.

@hisener hisener mentioned this pull request Dec 30, 2017
Closed
@hisener hisener changed the title Add argument conversions for Currency and URI Add argument conversions for Currency, Locale, and URI Dec 30, 2017
@hisener
Copy link
Contributor Author

hisener commented Dec 30, 2017

@marcphilipp I have updated Javadoc.

@sbrannen Added Locale as well.

@sbrannen sbrannen added this to the 5.1 M2 milestone Dec 30, 2017
@sbrannen
Copy link
Member

FYI: slated for 5.1 M2

@sbrannen
Copy link
Member

@hisener, can you please document the changes in the User Guide and Release Notes and update this PR?

Thanks!

@hisener
Copy link
Contributor Author

hisener commented Dec 30, 2017

@sbrannen 👍

@sbrannen sbrannen merged commit 1dc5c06 into junit-team:master Dec 30, 2017
@sbrannen
Copy link
Member

This has been merged into master for inclusion in 5.1.

Thanks!

@marcphilipp
Copy link
Member

Thanks, guys! 😀

@juergenzimmermann juergenzimmermann mentioned this pull request Jan 7, 2018
Closed
3 tasks
Andrei94 pushed a commit to Andrei94/junit5 that referenced this pull request Jun 23, 2018
@hisener @Andrei94
Add argument conversions for Currency, Locale, and URI (junit-team#1221)
bb7abbd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrannen sbrannen sbrannen left review comments

@jbduncan jbduncan jbduncan left review comments

Assignees
No one assigned
Labels
component: Jupiter theme: parameterized tests type: enhancement
Projects
None yet
Milestone
5.1 M2
Development

Successfully merging this pull request may close these issues.
4 participants
@hisener @marcphilipp @sbrannen @jbduncan