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

Add Period converter support #21136

Closed
wants to merge 9 commits into from
Closed

Conversation

Grubhart
Copy link
Contributor

@Grubhart Grubhart commented Apr 27, 2020

Added PeriodToString, PeriodToNumber, and StringToPeriod Classes

see gh-21080

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 27, 2020
Copy link
Contributor

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

@Grubhart good job! just left few small comments. Also, perform ./gradlew format command.


/**
* {@link Converter} to convert from a {@link Number} to a {@link Period}. Supports
* {@link Period#parse(CharSequence)} as well a more readable {@code 10s} form.
Copy link
Contributor

Choose a reason for hiding this comment

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

since Period doesn't support time I think we can change 10s to 10m

public abstract String print(Period value, ChronoUnit unit);

/**
* Detect the style then parse the value to return a duration.
Copy link
Contributor

Choose a reason for hiding this comment

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

change duration to period

/**
* Detect the style then parse the value to return a duration.
* @param value the value to parse
* @return the parsed duration
Copy link
Contributor

Choose a reason for hiding this comment

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

change duration to period

import static org.assertj.core.api.Assertions.assertThat;

/**
* Tests for {@link DurationToStringConverter}.
Copy link
Contributor

Choose a reason for hiding this comment

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

it is PeriodToStringConverter

}

public Period parse(String value) {

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

@mbhave mbhave changed the title Add Convert Period SUpport Add Period converter support Apr 29, 2020
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 2, 2020
@philwebb philwebb added this to the 2.3.x milestone May 2, 2020
@philwebb philwebb self-assigned this May 6, 2020
philwebb pushed a commit that referenced this pull request May 6, 2020
Add converter support for `javax.time.Period` including:

	String -> Period
	Number -> Period
	Period -> String

Period to Number conversion is not supported since `Period` has no
ability to deduce the number of calendar days in the period.

See gh-21136
philwebb added a commit that referenced this pull request May 6, 2020
Polish period converter support, primarily by changing
`PeriodStyle` to parse and print periods that include
more than one unit.

See gh-21136
@philwebb philwebb closed this in 66e8968 May 6, 2020
@philwebb
Copy link
Member

philwebb commented May 6, 2020

Thanks very much @Grubhart and @eddumelendez. I've managed to squeeze this into 2.3.0.

I have made some slight tweaks in commit 5ae623c to ensure that periods with more than one unit can be printed and parsed. I.e Period.of(1,2,3) will now print 1y2m3d and that value could be parsed again to give out back the same Period value.

@philwebb philwebb modified the milestones: 2.3.x, 2.3.0 May 6, 2020
/**
* Simple formatting, for example '1d'.
*/
SIMPLE("^([\\+\\-]?\\d+)([a-zA-Z]{0,2})$") {

Choose a reason for hiding this comment

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

I'm happy to see this! My little grain of sand.

You don't need to escape + and - (if it's at the end) in a character class

SIMPLE("^([+-]?\\d+)([a-zA-Z]{0,2})$")

Copy link
Member

Choose a reason for hiding this comment

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

That was accident that came about because I copy/pasted some code from Period :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll apply the same fix to the other patterns that we define.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants