Skip to content

Add Converters for Additional Types #47

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
Merged
garydgregory merged 1 commit into from
Feb 4, 2024
Merged

Add Converters for Additional Types #47

garydgregory merged 1 commit into from
Feb 4, 2024

Conversation

@SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Nov 3, 2020

This adds the following converters, with test cases:

  • Color
  • Dimension
  • InetAddress
  • Locale
  • Pattern
  • Point

You can see the test cases for example inputs/outputs.

I've just taken my PR from DeltaSpike (apache/deltaspike#109) and converted the converters that didn't exist here to work with this project instead. This is just me acting on a comment in the PR (apache/deltaspike#109 (comment)) in case it does happen, I figured I could have the PR here already.

This does not port all changes from my other PR, for example I implemented some existing converters differently, but I'll PR those separately, so they can just be closed if they're unfavorable.

melloware
melloware approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@melloware melloware left a comment

Choose a reason for hiding this comment

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

Very nice additions!

@melloware
Copy link
Contributor

melloware commented Nov 3, 2020

Definitely submit your PR of your other changes. I would like to see it and I did some of these converters and wasn't quite sure the best way to handle so i will be interested to see what you have done.

@SethFalco SethFalco mentioned this pull request Nov 4, 2020
Merged
@melloware
Copy link
Contributor

melloware commented Dec 30, 2020

Also rebase this one please.

@SethFalco
Copy link
Contributor Author

SethFalco commented Dec 30, 2020

Thanks for the comments, I believe both should be resolved now.
Let me know if there're any other issues.

garydgregory
garydgregory requested changes Dec 30, 2020
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hi @SethFalco

Thank you for your PR.

Please rebase on master and see my comments scattered throughout which apply to the whole PR.

G

garydgregory
garydgregory reviewed Dec 30, 2020
View reviewed changes
src/main/java/org/apache/commons/beanutils2/ConvertUtilsBean.java
// @formatter:off
register(Class.class, throwException ? new ClassConverter() : new ClassConverter(null));
register(ColorConverter.class, throwException ? new ColorConverter() : new ColorConverter(null));
register(Enum.class, throwException ? new EnumConverter() : new EnumConverter(null));
Copy link
Member

@garydgregory garydgregory Dec 30, 2020

Choose a reason for hiding this comment

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

Isn't the first argument supposed to be Color? If so, how are the tests passing?

@SethFalco SethFalco requested a review from garydgregory July 9, 2021 03:15
@SethFalco
Copy link
Contributor Author

SethFalco commented Jul 9, 2021

Sorry for the delay in coming back to this.
Ended up getting pretty busy, then put this off for a while.

  • I've rebased with master.
  • Squashed my commits.
  • I believe I've resolved all of your feedback.

High hopes everything is cool, but I'll be available if you have any further feedback.

@SethFalco SethFalco mentioned this pull request Oct 1, 2021
Closed
@SethFalco
Copy link
Contributor Author

SethFalco commented Apr 25, 2022

@garydgregory If this PR it too large to review at once, would you prefer if I split it up into smaller ones?

I could do one PR per converter for example?

@garydgregory
Copy link
Member

garydgregory commented Apr 25, 2022 via email

@SethFalco SethFalco force-pushed the add-converters branch 2 times, most recently from e475de6 to 2d058ef Compare February 1, 2024 19:21
@garydgregory
Copy link
Member

garydgregory commented Feb 1, 2024 •
edited
Loading

This is OK BUT I think we need to spit up this project into a multi-module Maven project because we are going to want to split out of module for code that depends on java.desktop at least. I know that's been a complaint in other Commons projects. I'll have a look in a couple of days.

@SethFalco
Copy link
Contributor Author

SethFalco commented Feb 1, 2024 •
edited
Loading

Thanks for taking a look! And makes sense, in DeltaSpike there were also comments regarding the awt packages.

Feel free to let me know how you want to proceed, and I don't mind dropping some of the converters from this PR.

(The last push just amends some of the comments/Javadocs btw.)

@garydgregory
Copy link
Member

garydgregory commented Feb 1, 2024

I don't think you need to do anything. I'll take a look later at how to best split things up.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024 •
edited
Loading

Codecov Report

❌ Patch coverage is 56.73759% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.11%. Comparing base (a31a210) to head (4babb4d).
⚠️ Report is 445 commits behind head on master.

Files with missing lines Patch % Lines
.../commons/beanutils2/converters/ColorConverter.java 65.00% 17 Missing and 4 partials ⚠️
.../commons/beanutils2/converters/PointConverter.java 47.61% 7 Missing and 4 partials ⚠️
...mons/beanutils2/converters/DimensionConverter.java 57.89% 5 Missing and 3 partials ⚠️
...rg/apache/commons/beanutils2/ConvertUtilsBean.java 50.00% 0 Missing and 6 partials ⚠️
...ns/beanutils2/converters/InetAddressConverter.java 54.54% 4 Missing and 1 partial ⚠️
...commons/beanutils2/converters/LocaleConverter.java 44.44% 4 Missing and 1 partial ⚠️
...ommons/beanutils2/converters/PatternConverter.java 44.44% 4 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master      #47      +/-   ##
============================================
- Coverage     65.33%   65.11%   -0.22%     
- Complexity     1488     1515      +27     
============================================
  Files           105      111       +6     
  Lines          5504     5645     +141     
  Branches       1068     1086      +18     
============================================
+ Hits           3596     3676      +80     
- Misses         1449     1490      +41     
- Partials        459      479      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@garydgregory
Copy link
Member

garydgregory commented Feb 1, 2024

@SethFalco
Run 'mvn' by itself to catch all these build errors.

@SethFalco
Copy link
Contributor Author

SethFalco commented Feb 1, 2024

My bad, I didn't realize mvn test didn't run Checkstyle.
Fixed, hopefully should be all good this time.

@garydgregory garydgregory merged commit a234761 into apache:master Feb 4, 2024
@SethFalco SethFalco deleted the add-converters branch February 4, 2024 15:21
@SethFalco SethFalco mentioned this pull request Apr 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garydgregory garydgregory Awaiting requested review from garydgregory

1 more reviewer

@melloware melloware melloware approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SethFalco @melloware @garydgregory @codecov-commenter