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 possibility to parse days and milliseconds with config DurationConverter #32929

Merged
merged 1 commit into from
May 20, 2023
Merged

Add possibility to parse days and milliseconds with config DurationConverter #32929

merged 1 commit into from
May 20, 2023

Conversation

manofthepeace
Copy link
Contributor

This adds the possibility to convert durations in milliseconds like 25ms or days 7d. Made the START_WITH_DIGITS case a bit more solid by allowing only valid units, case insensitive.

@geoand geoand requested a review from radcortez April 27, 2023 08:39
gsmet
gsmet approved these changes May 2, 2023
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I think it's an interesting addition but let's wait for @radcortez 's opinion before merging.

@quarkus-bot

This comment has been minimized.

Sign in to view
radcortez
radcortez requested changes May 2, 2023
View reviewed changes
Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

I'm ok with the new feature, but the test failures are related.

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@manofthepeace
Copy link
Contributor Author

rebased, just in case.

@manofthepeace
Copy link
Contributor Author

@gsmet do you think it is possible to have that in 3.1?

@manofthepeace manofthepeace mentioned this pull request May 12, 2023
Merged
machi1990
machi1990 requested changes May 16, 2023
View reviewed changes
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

Nice addition, thanks @manofthepeace . Can we update the duration format note doc: https://github.com/quarkusio/quarkus/blob/f7b67786ab8d51c499227bcceee523eb96298e08/docs/src/main/asciidoc/_includes/duration-format-note.adoc? Thanks

@manofthepeace manofthepeace force-pushed the durationConverterMoreUnits branch 2 times, most recently from 9bd6b69 to fee131f Compare May 16, 2023 12:45
@manofthepeace
Copy link
Contributor Author

Nice addition, thanks @manofthepeace . Can we update the duration format note doc: https://github.com/quarkusio/quarkus/blob/f7b67786ab8d51c499227bcceee523eb96298e08/docs/src/main/asciidoc/_includes/duration-format-note.adoc? Thanks

@machi1990 thanks for the review. Updated the duration format note.

@manofthepeace manofthepeace force-pushed the durationConverterMoreUnits branch 2 times, most recently from f414c6f to e774246 Compare May 16, 2023 12:57
@machi1990 machi1990 requested a review from radcortez May 16, 2023 21:57
@machi1990 machi1990 requested a review from gsmet May 16, 2023 21:57
@machi1990 machi1990 added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 17, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@manofthepeace
Copy link
Contributor Author

@machi1990 went though the log but have trouble seeing anything related, would you know better?

@machi1990
Copy link
Member

@machi1990 went though the log but have trouble seeing anything related, would you know better?

I've re-run the failed job again to rule out any consistent failures.

@quarkus-bot
Copy link

quarkus-bot bot commented May 20, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@machi1990 machi1990 merged commit a325036 into quarkusio:main May 20, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 20, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 20, 2023
@github-actions
Copy link

🙈 The PR is closed and the preview is expired.

@manofthepeace manofthepeace deleted the durationConverterMoreUnits branch May 23, 2023 13:05
@manofthepeace manofthepeace mentioned this pull request Jul 5, 2023
Merged
@Ladicek Ladicek mentioned this pull request Sep 11, 2023
Merged
@manofthepeace manofthepeace mentioned this pull request Feb 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radcortez radcortez radcortez approved these changes

@machi1990 machi1990 machi1990 approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/config area/core area/documentation
Projects
None yet
Milestone
3.2.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@manofthepeace @machi1990 @gsmet @radcortez