You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltaspike.apache.org by GitBox <gi...@apache.org> on 2020/08/06 21:55:49 UTC
[GitHub] [deltaspike] SethiPandi opened a new pull request #109: Appended 2 Small Add-Ons for DeltaSpike to Documentation
SethiPandi opened a new pull request #109:
URL: https://github.com/apache/deltaspike/pull/109
Since we're using DeltaSpike we've ended up making some libraries to add a little on top, if it's alright, I thought they could be put in the `Add-ons` section of the documentation.
These libraries are rather small, but do work, are being maintained, and are unit tested.
---
As a side note, there appears to be some redundancy in the documentation.
There is a dedicated "Add-ons" page here:
https://deltaspike.apache.org/addons.html
But there is also an "Add-ons" heading under "External Resources" here:
https://deltaspike.apache.org/external.html#_add_ons
Is this intentional? I think it may be better if the Add-ons section was removed from External Resources, and the information conveyed there could be merged with the dedicated Add-ons page?
This allows the Add-ons documentation to be centralized rather than spread across the documentation.
(It's a minimal change, so with consent I could just do it in this PR.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi edited a comment on pull request #109: Appended 2 Small Add-Ons for DeltaSpike to Documentation
Posted by GitBox <gi...@apache.org>.
SethiPandi edited a comment on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-670228351
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] rmannibucau commented on a change in pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#discussion_r720517626
##########
File path: deltaspike/core/impl/src/main/java/org/apache/deltaspike/core/impl/config/TypedResolverImpl.java
##########
@@ -504,6 +530,66 @@ else if (Double.class.equals(configEntryType))
{
result = Double.parseDouble(value);
}
+ else if (Character.class.equals(configEntryType))
Review comment:
think we must take care of the packages, we likely don't want awt package to be loaded there because there is a security manager or javaagent for example.
think it can be better to provide the classes and enable to provide a Map<Class<?>, Converter<?>> to the resolver.
by default - for implicit resolvers - we can try to load all the ones we support - even if it should be per package group likely to avoid to fail 4 times when once is enough?. This default loading should likely be done in the config extension or something like that and cached to be reused/reusable (if the extension is injected). Manual resolvers can be loaded without any additional converter (primitives/wrappers being the one we must support OOTB only IMHO).
Side note: for the story behind this way to do, we did the exact same thing in xbean with its converts == load them all by default, but in practise it is very bothering and letting them be configurable and loaded at demand only is way more relevant on user land in general.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] tandraschko commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
tandraschko commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-698827874
looks very good to me!
@struberg can we merge it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] tandraschko commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
tandraschko commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-698827874
looks very good to me!
@struberg can we merge it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethiPandi commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-671145322
I've gone ahead and moved all the code from my project into DeltaSpike Core Impl module, and updated the code styles to match this repository.
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 for a PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi edited a comment on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethiPandi edited a comment on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-720465974
Since opening this PR, I've noticed Apache BeanUtils has lots of converters similar to how this project does. :thinking:
I was just wondering, since altogether it'd be great to have more converters and some code is quite similar, though the general structure and interface is a bit different.
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi edited a comment on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethiPandi edited a comment on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-671145322
I've gone ahead and moved all the code from one of the projects into DeltaSpike Core Impl module for this PR, and updated the code styles to match this repository.
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 for a PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethiPandi commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-720465974
Since opening this PR, I've noticed Apache BeanUtils has lots of converters similar to how this project does. :thinking:
I was just wondering, since altogether it'd be great to have more converters and some code is quite similar, though the general structure and interface is a bit different.
Would it be feasible to pull them both into a separate project and just have both DeltaSpike and BeanUtils depend on it?
https://github.com/apache/commons-beanutils/tree/master/src/main/java/org/apache/commons/beanutils2/converters
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethFalco commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethFalco commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-932497792
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](https://github.com/apache/commons-beanutils/pull/47#discussion_r550225810))
* 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](https://github.com/apache/commons-beanutils/pull/49#discussion_r550578710))
* Updated tests for `URL` to compare the `URI` instead to avoid depending on an internet connection.
* Allow British and American spellings for `Color`. ([Comment](https://github.com/apache/commons-beanutils/pull/47#discussion_r550229164))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethFalco edited a comment on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethFalco edited a comment on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-927151911
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.
* [Duration](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Duration.html) ([`Duration#parse`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Duration.html#parse(java.lang.CharSequence)))
* [File](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/File.html) ([`File::new`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/File.html#%3Cinit%3E(java.lang.String)))
* [Instant](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html) ([`Instant#parse`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html#parse(java.lang.CharSequence)))
* [Period](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Period.html) ([`Period#parse`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Period.html#parse(java.lang.CharSequence)))
* [URI](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/URI.html) ([`URI::new`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/URI.html#%3Cinit%3E(java.lang.String)))
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:
* [UUID](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/UUID.html) ([`UUID#fromString`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/UUID.html#fromString(java.lang.String)))
* [PosixFilePermissions](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/attribute/PosixFilePermissions.html) ([`PosixFilePermissions#fromString`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/attribute/PosixFilePermissions.html#fromString(java.lang.String)))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi commented on pull request #109: Appended 2 Small Add-Ons for DeltaSpike to Documentation
Posted by GitBox <gi...@apache.org>.
SethiPandi commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-670228351
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 contributing into this repository, l gladly give it a shot to move them over.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] tandraschko commented on pull request #109: Appended 2 Small Add-Ons for DeltaSpike to Documentation
Posted by GitBox <gi...@apache.org>.
tandraschko commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-670218175
In general i wonder why dont you contribute directly to DeltaSpike?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethFalco commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethFalco commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-882034516
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 seperate PR here.
* If DeltaSpike **won't** depend on BeanUtils: I'll rebase rebase/update this to make improvements or match feedback from BeanUtils.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] struberg commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
struberg commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-899075374
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethiPandi edited a comment on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethiPandi edited a comment on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-671145322
I've gone ahead and moved all the code from one of the projects into DeltaSpike Core Impl module for this PR, and updated the code styles to match this repository.
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] tandraschko commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
tandraschko commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-720469005
Not sure if both projects are interessing in such a shared codebase. Couldnt we reuse beanutils here? cc @struberg
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethFalco commented on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethFalco commented on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-927151911
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.
* [Duration](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Duration.html) ([`Duration#parse`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Duration.html#parse(java.lang.CharSequence)))
* [File](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/File.html) ([`File::new`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/File.html#%3Cinit%3E(java.lang.String)))
* [Instant](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html) ([`Instant#parse`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html#parse(java.lang.CharSequence)))
* [Period](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Period.html) ([`Period#parse`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Period.html#parse(java.lang.CharSequence)))
* [URI](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/URI.html) ([`URI::new`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/URI.html#%3Cinit%3E(java.lang.String)))
Just a note, but something that could've been implicit, but doesn't meet the current spec is `#fromString`?
But only the following have it in the standard library:
* [UUID](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/UUID.html) ([`UUID#fromString`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/UUID.html#fromString(java.lang.String)))
* [PosixFilePermissions](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/attribute/PosixFilePermissions.html) ([`PosixFilePermissions#fromString`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/attribute/PosixFilePermissions.html#fromString(java.lang.String)))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [deltaspike] SethFalco edited a comment on pull request #109: Support for Additional Converters by Default
Posted by GitBox <gi...@apache.org>.
SethFalco edited a comment on pull request #109:
URL: https://github.com/apache/deltaspike/pull/109#issuecomment-882034516
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@deltaspike.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org