deltaspike icon indicating copy to clipboard operation
deltaspike copied to clipboard

Support for Additional Converters by Default

Open SethFalco opened this issue 4 years ago • 9 comments

Adds default converts for the following types:

  • java.awt.Color
  • java.awt.Dimension
  • java.awt.Point
  • java.io.File
  • java.net.InetAddress
  • java.net.URI
  • java.net.URL
  • java.time.Duration
  • java.time.Instant
  • java.time.Period
  • java.util.Locale
  • java.util.UUID
  • java.util.regex.Pattern

Edit: Originally a request to add a plugin to the docs, but made this a contribution instead.

SethFalco avatar Aug 06 '20 21:08 SethFalco

In general i wonder why dont you contribute directly to DeltaSpike?

tandraschko avatar Aug 06 '20 22:08 tandraschko

I could try to do that if it's alright. I have no problem with committing code in the DeltaSpike project, in fact, I'd prefer it.

I just kept it separate as I needed it for another project and didn't feel familiar enough with the DeltaSpike project internally to feel confident contributing directly.

If you've seen them and believe they could, and should be contributed into this repository, l gladly give it a shot to move them over.

SethFalco avatar Aug 06 '20 22:08 SethFalco

I've moved everything from the proposed plugin into the DeltaSpike Core Impl module.

It includes implementations for several standard Java types, tests for each converter, and a small documentation update to mention the additional supported types.

Does this look alright so far for a PR?

SethFalco avatar Aug 10 '20 03:08 SethFalco

looks very good to me! @struberg can we merge it?

tandraschko avatar Sep 25 '20 09:09 tandraschko

Since opening this PR, I've noticed Apache BeanUtils has many converters similar to how this project does. :thinking:

Would it be feasible to have a general "converters" project, and have both DeltaSpike and BeanUtils depend on it?

https://github.com/apache/commons-beanutils/tree/master/src/main/java/org/apache/commons/beanutils2/converters

SethFalco avatar Nov 02 '20 13:11 SethFalco

Not sure if both projects are interessing in such a shared codebase. Couldnt we reuse beanutils here? cc @struberg

tandraschko avatar Nov 02 '20 13:11 tandraschko

If possible, it could be nice to get an answer on this. I'm hoping the pull request to commons-beanutils could be merged soon.

  • If DeltaSpike may depend on BeanUtils: we can close this, and I may look if I can add BeanUtils in a separate PR here.
  • If DeltaSpike won't depend on BeanUtils: I'll rebase/update this to make improvements or match feedback from BeanUtils.

SethFalco avatar Jul 18 '21 10:07 SethFalco

Hi Seth! I'd rather not depend on BeanUtils. The less dependencies we do have, the better it is imo. The code for picking up the converters is rather trivial.

That said, what do you think about doing something similar than we did when implementing mp-config over in Geronimo: implicit converters! https://github.com/apache/geronimo-config/blob/ConfigJSR/impl/src/main/java/org/apache/geronimo/config/converters/ImplicitConverter.java

I've also added this to the ConfigJSR proposal: https://github.com/eclipse/ConfigJSR/blob/master/spec/src/main/asciidoc/converters.asciidoc#implicit-converters With that we'd need to add Converters only for those classes where the implicit converter rules do not already apply. Wdyt?

struberg avatar Aug 15 '21 16:08 struberg

Hey! Understood, I'll squash this PR and match the improvements suggested from BeanUtils. Even if this won't be merged as is, the logic should still be updated anyway.

Meanwhile, having implicit converters seems cool.

This PR includes 5 converters that would be made redundant, assuming we stuck with the current implementations.

Just a note, but something that could've been implicit, but doesn't meet the current spec is #fromString(java.lang.String)? But only the following have it in the standard library:

SethFalco avatar Sep 25 '21 17:09 SethFalco

Just finished cleaning this up, a summary of the changes:

  • Squashed the commits.
  • Reduced the verbosity of the docs/errors. (Semantics are the same, they were just really wordy.)
  • Used more concise syntax for testing exceptions. (@Test(expected = {}) (Comment)
  • Updated @since 1.9.5 to @since 1.9.6 since 1.9.5 has already been released.
  • Duration, Instant, and Period no longer accept Long values by default. (Comment)
  • Updated tests for URL to compare the URI instead to avoid depending on an internet connection.
  • Allow British and American spellings for Color. (Comment)

SethFalco avatar Oct 01 '21 19:10 SethFalco