DefaultConversionAnnotationProcessor and ConversionRule (usage?)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

DefaultConversionAnnotationProcessor and ConversionRule (usage?)

Yasser Zamani-2
Hi there, happy new year :)

During resolving WW-4906, I reached following difficulties with these
two classes and I need your comments if any, please:

DefaultConversionAnnotationProcessor

>     public void process(Map<String, Object> mapping, TypeConversion tc, String key) {
>         ...
>         try {
>             if (tc.type() == ConversionType.APPLICATION) {
>                 ...
>             } else {
>                 if (tc.rule() == ConversionRule.KEY_PROPERTY || tc.rule() == ConversionRule.CREATE_IF_NULL) {

If CREATE_IF_NULL and KEY_PROPERTY have same behavior, why they are two?
What makes difference?

>                     mapping.put(key, tc.value());

I could not find any sample or doc about usage and advantages of
specifying value explicitly.

>                 }
>                 //for properties of classes
>                 else if (tc.rule() != ConversionRule.ELEMENT || tc.rule() == ConversionRule.KEY || tc.rule() == ConversionRule.COLLECTION) {

`tc.rule() != ConversionRule.ELEMENT` was enough i.e. next two ORs don't
make any difference. Or maybe it's wrong and next two ORs should being
keeped?

>                     ...
>                 }
>                 //for keys of Maps
>                 else if (tc.rule() == ConversionRule.KEY) {

Execution never reaches inside this block. Here 100% tc.rule() is
ConversionRule.ELEMENT because of previous if. And also why previous if
also has this in it's ORs?

Again, I could not find any sample or doc about usage and advantages of
this block! (following lines).

>                     Class<?> converterClass;
>                     if (StringUtils.isNoneEmpty(tc.converter())) {
>                         converterClass = Thread.currentThread().getContextClassLoader().loadClass(tc.converter());
>                         //check if the converter is a type converter if it is one
>                         //then just put it in the map as is. Otherwise
>                         //put a value in for the type converter of the class
>                     } else {
>                         converterClass = tc.converterClass();
>                     }
>
>                     LOG.debug("Converter class: [{}]", converterClass);
>
>                     if (converterClass.isAssignableFrom(TypeConverter.class)) {
>                         mapping.put(key, converterCreator.createTypeConverter(tc.converter()));
>                     } else {
>                         mapping.put(key, converterClass);
>                         LOG.debug("Object placed in mapping for key [{}] is [{}]", key, mapping.get(key));
>                     }
>                 }
>                 //elements(values) of maps / lists
>                 else {

Again, I could not find any sample or doc about usage and advantages of
this block! (following lines).

>                     if (StringUtils.isNoneEmpty(tc.converter())) {
>                         mapping.put(key, Thread.currentThread().getContextClassLoader().loadClass(tc.converter()));
>                     } else {
>                         mapping.put(key, tc.converterClass());
>                     }
>                 }
>             }
>         } catch (Exception e) {
>             LOG.debug("Got exception for {}", key, e);
>         }
>     }


ConversionRule

> public enum ConversionRule {
>
>     PROPERTY, COLLECTION, MAP, KEY, KEY_PROPERTY, ELEMENT, CREATE_IF_NULL;

Again, I could not find any comprehensive sample or doc about usage and
advantages of each one!


Thanks in advance!


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultConversionAnnotationProcessor and ConversionRule (usage?)

Lukasz Lenart
As far I understand these annotations are representation of the same
properties which can be used in XXX-conversion.properties files
https://struts.apache.org/core-developers/type-conversion.html#collection-and-map-support


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

2018-01-03 19:42 GMT+01:00 Yasser Zamani <[hidden email]>:

> Hi there, happy new year :)
>
> During resolving WW-4906, I reached following difficulties with these
> two classes and I need your comments if any, please:
>
> DefaultConversionAnnotationProcessor
>
>>     public void process(Map<String, Object> mapping, TypeConversion tc, String key) {
>>         ...
>>         try {
>>             if (tc.type() == ConversionType.APPLICATION) {
>>                 ...
>>             } else {
>>                 if (tc.rule() == ConversionRule.KEY_PROPERTY || tc.rule() == ConversionRule.CREATE_IF_NULL) {
>
> If CREATE_IF_NULL and KEY_PROPERTY have same behavior, why they are two?
> What makes difference?
>
>>                     mapping.put(key, tc.value());
>
> I could not find any sample or doc about usage and advantages of
> specifying value explicitly.
>
>>                 }
>>                 //for properties of classes
>>                 else if (tc.rule() != ConversionRule.ELEMENT || tc.rule() == ConversionRule.KEY || tc.rule() == ConversionRule.COLLECTION) {
>
> `tc.rule() != ConversionRule.ELEMENT` was enough i.e. next two ORs don't
> make any difference. Or maybe it's wrong and next two ORs should being
> keeped?
>
>>                     ...
>>                 }
>>                 //for keys of Maps
>>                 else if (tc.rule() == ConversionRule.KEY) {
>
> Execution never reaches inside this block. Here 100% tc.rule() is
> ConversionRule.ELEMENT because of previous if. And also why previous if
> also has this in it's ORs?
>
> Again, I could not find any sample or doc about usage and advantages of
> this block! (following lines).
>
>>                     Class<?> converterClass;
>>                     if (StringUtils.isNoneEmpty(tc.converter())) {
>>                         converterClass = Thread.currentThread().getContextClassLoader().loadClass(tc.converter());
>>                         //check if the converter is a type converter if it is one
>>                         //then just put it in the map as is. Otherwise
>>                         //put a value in for the type converter of the class
>>                     } else {
>>                         converterClass = tc.converterClass();
>>                     }
>>
>>                     LOG.debug("Converter class: [{}]", converterClass);
>>
>>                     if (converterClass.isAssignableFrom(TypeConverter.class)) {
>>                         mapping.put(key, converterCreator.createTypeConverter(tc.converter()));
>>                     } else {
>>                         mapping.put(key, converterClass);
>>                         LOG.debug("Object placed in mapping for key [{}] is [{}]", key, mapping.get(key));
>>                     }
>>                 }
>>                 //elements(values) of maps / lists
>>                 else {
>
> Again, I could not find any sample or doc about usage and advantages of
> this block! (following lines).
>
>>                     if (StringUtils.isNoneEmpty(tc.converter())) {
>>                         mapping.put(key, Thread.currentThread().getContextClassLoader().loadClass(tc.converter()));
>>                     } else {
>>                         mapping.put(key, tc.converterClass());
>>                     }
>>                 }
>>             }
>>         } catch (Exception e) {
>>             LOG.debug("Got exception for {}", key, e);
>>         }
>>     }
>
>
> ConversionRule
>
>> public enum ConversionRule {
>>
>>     PROPERTY, COLLECTION, MAP, KEY, KEY_PROPERTY, ELEMENT, CREATE_IF_NULL;
>
> Again, I could not find any comprehensive sample or doc about usage and
> advantages of each one!
>
>
> Thanks in advance!
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultConversionAnnotationProcessor and ConversionRule (usage?)

Yasser Zamani-2


On 1/4/2018 10:58 AM, Lukasz Lenart wrote:
> As far I understand these annotations are representation of the same
> properties which can be used in XXX-conversion.properties files
> https://struts.apache.org/core-developers/type-conversion.html#collection-and-map-support

Thanks Łukasz! With further investigation following above link, I
discovered those codes are copied from `DefaultConversionFileProcessor`.
Then I saw in it ...

>                     //for properties of classes
>                     else if (!(key.startsWith(DefaultObjectTypeDeterminer.ELEMENT_PREFIX) ||
>                             key.startsWith(DefaultObjectTypeDeterminer.KEY_PREFIX) ||
>                             key.startsWith(DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX))
>                             ) {

i.e. all ORs are NOTed. Now I understand what's the problem with
`DefaultConversionAnnotationProcessor` :) Copier of those codes wished
to remove NOT but wrongly just applied on first condition and rewrote it
as tc.rule() != ConversionRule.ELEMENT while `NOT` should be refactored
to all inside () i.e. tc.rule() != ConversionRule.ELEMENT && tc.rule()
!= ConversionRule.KEY && tc.rule() != ConversionRule.COLLECTION.

Also I found a lot of XXX-conversion.properties inside test resources
and now, I could coveralls my PR DefaultConversionAnnotationProcessor
without knowing underlying details via copying current tests ;)

Regards.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultConversionAnnotationProcessor and ConversionRule (usage?)

Lukasz Lenart
Just one note: if you do plan a large refactoring I would suggest to
split it into two steps. In first step remove the @deprecated flag and
push changes, then we can easily release 2.5.15. And postpone the
second step (the refactoring) till 2.6.

I do not expect large changes in 2.5.x codebase


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

2018-01-04 11:56 GMT+01:00 Yasser Zamani <[hidden email]>:

>
>
> On 1/4/2018 10:58 AM, Lukasz Lenart wrote:
>> As far I understand these annotations are representation of the same
>> properties which can be used in XXX-conversion.properties files
>> https://struts.apache.org/core-developers/type-conversion.html#collection-and-map-support
>
> Thanks Łukasz! With further investigation following above link, I
> discovered those codes are copied from `DefaultConversionFileProcessor`.
> Then I saw in it ...
>
>>                     //for properties of classes
>>                     else if (!(key.startsWith(DefaultObjectTypeDeterminer.ELEMENT_PREFIX) ||
>>                             key.startsWith(DefaultObjectTypeDeterminer.KEY_PREFIX) ||
>>                             key.startsWith(DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX))
>>                             ) {
>
> i.e. all ORs are NOTed. Now I understand what's the problem with
> `DefaultConversionAnnotationProcessor` :) Copier of those codes wished
> to remove NOT but wrongly just applied on first condition and rewrote it
> as tc.rule() != ConversionRule.ELEMENT while `NOT` should be refactored
> to all inside () i.e. tc.rule() != ConversionRule.ELEMENT && tc.rule()
> != ConversionRule.KEY && tc.rule() != ConversionRule.COLLECTION.
>
> Also I found a lot of XXX-conversion.properties inside test resources
> and now, I could coveralls my PR DefaultConversionAnnotationProcessor
> without knowing underlying details via copying current tests ;)
>
> Regards.
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultConversionAnnotationProcessor and ConversionRule (usage?)

Yasser Zamani-2


On 1/6/2018 11:36 AM, Lukasz Lenart wrote:

> Just one note: if you do plan a large refactoring I would suggest to
> split it into two steps. In first step remove the @deprecated flag and
> push changes, then we can easily release 2.5.15. And postpone the
> second step (the refactoring) till 2.6.
>
> I do not expect large changes in 2.5.x codebase
>
>
> Regards
> -- Łukasz + 48 606 323 122 http://www.lenart.org.pl/ 2018-01-04 11:56
> GMT+01:00 Yasser Zamani <[hidden email]>:
>>
>> On 1/4/2018 10:58 AM, Lukasz Lenart wrote:
>>> As far I understand these annotations are representation of the same
>>> properties which can be used in XXX-conversion.properties files
>>> https://struts.apache.org/core-developers/type-conversion.html#collection-and-map-support
>> Thanks Łukasz! With further investigation following above link, I
>> discovered those codes are copied from `DefaultConversionFileProcessor`.
>> Then I saw in it ...
>>
>>>                      //for properties of classes
>>>                      else if (!(key.startsWith(DefaultObjectTypeDeterminer.ELEMENT_PREFIX) ||
>>>                              key.startsWith(DefaultObjectTypeDeterminer.KEY_PREFIX) ||
>>>                              key.startsWith(DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX))
>>>                              ) {
>> i.e. all ORs are NOTed. Now I understand what's the problem with
>> `DefaultConversionAnnotationProcessor`:)  Copier of those codes wished
>> to remove NOT but wrongly just applied on first condition and rewrote it
>> as tc.rule() != ConversionRule.ELEMENT while `NOT` should be refactored
>> to all inside () i.e. tc.rule() != ConversionRule.ELEMENT && tc.rule()
>> != ConversionRule.KEY && tc.rule() != ConversionRule.COLLECTION.
>>
>> Also I found a lot of XXX-conversion.properties inside test resources
>> and now, I could coveralls my PR DefaultConversionAnnotationProcessor
>> without knowing underlying details via copying current tests;)

I just planned to fix above wrong if-else statement via inspiring from
DefaultConversionFileProcessor.java which is a small fix by itself but
also planned to add unit tests which currently does not exist but I knew
how via current tests. I conclude they're nice and not complex changes I
think ...

... But unfortunately I was and am busy and overloaded here these few
days (to make some money) ;) but I think I can finish this at end of
tomorrow then will review PR#196(WW-4905) next day and I think I'm ready
for 2.5.15 at another next day :)

Do you still prefer just nu-deprecating and leaving the bug as is? If
you confirm, then it's also ok at my side :)

Thanks for your attention,
Regards.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultConversionAnnotationProcessor and ConversionRule (usage?)

Lukasz Lenart
Ach... that's ok :) And take your time and relax, no hurry :)

2018-01-06 9:54 GMT+01:00 Yasser Zamani <[hidden email]>:

>
>
> On 1/6/2018 11:36 AM, Lukasz Lenart wrote:
>> Just one note: if you do plan a large refactoring I would suggest to
>> split it into two steps. In first step remove the @deprecated flag and
>> push changes, then we can easily release 2.5.15. And postpone the
>> second step (the refactoring) till 2.6.
>>
>> I do not expect large changes in 2.5.x codebase
>>
>>
>> Regards
>> -- Łukasz + 48 606 323 122 http://www.lenart.org.pl/ 2018-01-04 11:56
>> GMT+01:00 Yasser Zamani <[hidden email]>:
>>>
>>> On 1/4/2018 10:58 AM, Lukasz Lenart wrote:
>>>> As far I understand these annotations are representation of the same
>>>> properties which can be used in XXX-conversion.properties files
>>>> https://struts.apache.org/core-developers/type-conversion.html#collection-and-map-support
>>> Thanks Łukasz! With further investigation following above link, I
>>> discovered those codes are copied from `DefaultConversionFileProcessor`.
>>> Then I saw in it ...
>>>
>>>>                      //for properties of classes
>>>>                      else if (!(key.startsWith(DefaultObjectTypeDeterminer.ELEMENT_PREFIX) ||
>>>>                              key.startsWith(DefaultObjectTypeDeterminer.KEY_PREFIX) ||
>>>>                              key.startsWith(DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX))
>>>>                              ) {
>>> i.e. all ORs are NOTed. Now I understand what's the problem with
>>> `DefaultConversionAnnotationProcessor`:)  Copier of those codes wished
>>> to remove NOT but wrongly just applied on first condition and rewrote it
>>> as tc.rule() != ConversionRule.ELEMENT while `NOT` should be refactored
>>> to all inside () i.e. tc.rule() != ConversionRule.ELEMENT && tc.rule()
>>> != ConversionRule.KEY && tc.rule() != ConversionRule.COLLECTION.
>>>
>>> Also I found a lot of XXX-conversion.properties inside test resources
>>> and now, I could coveralls my PR DefaultConversionAnnotationProcessor
>>> without knowing underlying details via copying current tests;)
>
> I just planned to fix above wrong if-else statement via inspiring from
> DefaultConversionFileProcessor.java which is a small fix by itself but
> also planned to add unit tests which currently does not exist but I knew
> how via current tests. I conclude they're nice and not complex changes I
> think ...
>
> ... But unfortunately I was and am busy and overloaded here these few
> days (to make some money) ;) but I think I can finish this at end of
> tomorrow then will review PR#196(WW-4905) next day and I think I'm ready
> for 2.5.15 at another next day :)
>
> Do you still prefer just nu-deprecating and leaving the bug as is? If
> you confirm, then it's also ok at my side :)
>
> Thanks for your attention,
> Regards.
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]