Updated Existing Converters

[SethFalco]

This updates existing converters to the implementations referenced in #47. All existing test cases pass, and new ones have been added.

Character

Accepts hexadecimal representation for characters, to specify a character with its character code.

(I've worked somewhere where we couldn't put £ or € in the source. ^-^')

Enum

Accepts types of the Enum class, for example instead of assigning to the raw Enum type, it can accept input from java.time.DayOfWeek#MONDAY or java.time.DayOfWeek.MONDAY to get the constant MONDAY from the DayOfWeek enum.

Instant

Can now convert Instants (another chrono type).

---

• I've updated test classes for the converters that this affects to JUnit 5.

[melloware]

Really good improvements IMHO.

[melloware]

Looks like your EnumConverter test is failing the build?

[SethFalco]

Looks like your EnumConverter test is failing the build?

Ahh shoot, I'll fix that then. 🤔
Not sure, but this could be because I have JDK 11 as it seems to build/test for me locally. I'll swap out and see if it makes a difference.

---

I actually just noticed a small differences between the old and new EnumConverter, though.
The original was case-insensitive, but the new one is case-sensitive.

I was going to change it back, but want to verify something first, as that might not be ideal:

public enum SpecialEnum {
    HELLO,
    hello
}

Despite the fact it breaches naming conventions, it's technically still possible to have multiple enum constants with the same name if we ignore case, so I think it's better to be case-sensitive to avoid choosing the wrong one.

I think changing it to case-sensitive might be considered a breaking change, though?
Do you have any thoughts on if I should leave it case-sensitive, or revert it to be non-case sensitive?

[melloware]

I think if you look a the history Enum is a new converter for 2.0 which has not been released and never existing in 1.9.X branch so you should be OK and it shouldn't be a breaking change.

[SethFalco]

Not 100% sure why, but seems on Java 8 I need to add a cast to Enum. 🤔

[melloware]

Can you bump your Jacoco in pom.xml 0.8.6 so it fixes the build and retriggers please?

[SethFalco]

I've rebased the PR with master now, it should be up to date including the JaCoCo 0.8.6 commit.

[garydgregory]

@SethFalco
Thank you for your PR. This PR is almost to big. Please see my comments.

[garydgregory]

@SethFalco
Thank you for your PR. This PR is almost too big. Please see my comments. It might be simpler, depending on your reply, of splitting up this PR into smaller ones which would expedite non-controversial changes like the addition of new converters vs. changing existing ones.
G.

[mohanaraosv]

@garydgregory Some of your comments related to code format could be resolved by defining IDE specific code formatter. WDYT.

[melloware]

Yes that is why I have added a PR here to implement Spotless plugin so the code stays formatted to project standards no matter what IDE is used!

[SethFalco]

@garydgregory Some of your comments related to code format could be resolved by defining IDE specific code formatter. WDYT.

What @melloware suggested is much more appealing in general, coming from an IntelliJ user who is aware some other contributors are using Eclipse. It'd be better as a Maven/Gradle build task, Travis/GitHub action, or anything that does not bind the contributor to a particular IDE.

[garydgregory]

@garydgregory Some of your comments related to code format could be resolved by defining IDE specific code formatter. WDYT.

What @melloware suggested is much more appealing in general, coming from an IntelliJ user who is aware some other contributors are using Eclipse. It'd be better as a Maven/Gradle build task, Travis/GitHub action, or anything that does not bind the contributor to a particular IDE.

I get it but we would have to provide a consistent approach for all components in Apache Commons IMO. We could start with just one. Right now we use Checkstyle, so we would have to convert each component's checkstyle configuration to a new system. Yes, ideally we could have one rule set for all of Commons but we don't beyond use spaces not tabs.

Anyway, please feel free to bring this up on the dev mailing list.

[SethFalco]

I've rebased with master, squashed my commits, and resolved all feedback. High hopes everything is cool, but I'll be available if you have any other comments.

I opted to stick to updating this PR for now rather than split it, since it's getting smaller anyway. (Let me know if that's a problem!)

I left some changes in converters I reverted, for example:

• updating test cases
• applying conventions to nearby code to what I was modifying

[SethFalco]

I've rebased this pull request with master and resolved merge conflicts. Running mvn passes for me locally.

I'd also migrated the test cases modified in this PR to JUnit 5, as that API is already being used in other files in the repository.

Reference:

• Start migration to JUnit 5 #93

Edit: And I've rebased again following the merge of #47

[garydgregory]

Hi @SethFalco
Thank you for your update. Please see minor scattered comments.

[SethFalco]

Thanks for the review. I've rebased and made all requested changes.

Edit: Just rebased and re-requested review. My bad, I have a bad habit of posting a comment instead of using the re-request review button.