Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6217

fix warnings in trunk on java source code

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: ALL COMPONENTS
    • Labels:

      Description

      Right now, we have 528 warnings on trunk out of which 238 are about raw types and 118 never used imports. So we can already eliminate most of the warning quite quickly.

      I will issue multiple patches to resolve most of these warnings. It might be a bit of a challenge to eliminate the raw types because the generics are not always deducable from the code especially when relying on external APIs

      1. remove_unused_imports.patch
        55 kB
        Taher Alkhateeb
      2. warnings_patch_2.patch
        39 kB
        Taher Alkhateeb
      3. warnings_patch_2.patch
        39 kB
        Taher Alkhateeb
      4. OFBIZ-6217-patch-4.patch
        82 kB
        Taher Alkhateeb

        Activity

        Hide
        taher Taher Alkhateeb added a comment -

        partially implemented

        Show
        taher Taher Alkhateeb added a comment - partially implemented
        Hide
        taher Taher Alkhateeb added a comment -

        Okay. Closing this issue then.

        Show
        taher Taher Alkhateeb added a comment - Okay. Closing this issue then.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        I see a number of problems with the latest patch and I don't feel confident applying it. I prefer to wait for Adam to merge his fixes to the trunk, then we can resume from there.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - I see a number of problems with the latest patch and I don't feel confident applying it. I prefer to wait for Adam to merge his fixes to the trunk, then we can resume from there.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I had no special advices, just a global one, seems OK with your and Adrian's comments

        Show
        jacques.le.roux Jacques Le Roux added a comment - I had no special advices, just a global one, seems OK with your and Adrian's comments
        Hide
        doogie Adam Heath added a comment -

        Don't use UtilGenerics.cast() on an external lib call, if there is an update for that external lib that adds generics support. Upgrading that external lib to a new generics-capable version is outside the scope of this issue, so in that case, just ignore the warnings.

        File a separate issue, asking to get that external lib upgraded. Cross-link the 2 issues. When the lib is upgraded, then proceed with fixing the warnings(if they exist). But, in theory, the other new issue would hit upon changes to the generics markup, so would need to fix it there.

        As for the deprecation warnings, I've fixed all simple ones, except for any non-macro widget rendering. I've made a branch with all my changes. Please see the OFBIZ_6275 issue that Jacques mentioned.

        Show
        doogie Adam Heath added a comment - Don't use UtilGenerics.cast() on an external lib call, if there is an update for that external lib that adds generics support. Upgrading that external lib to a new generics-capable version is outside the scope of this issue, so in that case, just ignore the warnings. File a separate issue, asking to get that external lib upgraded. Cross-link the 2 issues. When the lib is upgraded, then proceed with fixing the warnings(if they exist). But, in theory, the other new issue would hit upon changes to the generics markup, so would need to fix it there. As for the deprecation warnings, I've fixed all simple ones, except for any non-macro widget rendering. I've made a branch with all my changes. Please see the OFBIZ_6275 issue that Jacques mentioned.
        Hide
        taher Taher Alkhateeb added a comment -

        Hey guys, thank you for the heads up.

        Adam what should I be careful about in terms of UtilGenerics.cast() and would you say it would be ill advised to convert Java built-in type (casting) to UtilGenerics.cast()? Would you mind taking a quick look at the latest patch and giving me your opinion where I might do things differently?

        Jacques, OFBIZ-6275 seems to focus on deprecated method calls. I was not able to touch the deprecated warnings yet because they were difficult and require advanced knowledge in the core framework and/or external libraries which I am unable to debug without first downloading the sources to. Do you have any hints where I should be more careful / alert?

        Show
        taher Taher Alkhateeb added a comment - Hey guys, thank you for the heads up. Adam what should I be careful about in terms of UtilGenerics.cast() and would you say it would be ill advised to convert Java built-in type (casting) to UtilGenerics.cast()? Would you mind taking a quick look at the latest patch and giving me your opinion where I might do things differently? Jacques, OFBIZ-6275 seems to focus on deprecated method calls. I was not able to touch the deprecated warnings yet because they were difficult and require advanced knowledge in the core framework and/or external libraries which I am unable to debug without first downloading the sources to. Do you have any hints where I should be more careful / alert?
        Hide
        doogie Adam Heath added a comment -

        And, be very very very careful about anything to do with generics. Use of the UtilGenerics.cast() should be extremely minimized.

        Show
        doogie Adam Heath added a comment - And, be very very very careful about anything to do with generics. Use of the UtilGenerics.cast() should be extremely minimized.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Maybe unrelated but beware of OFBIZ-6275

        Show
        jacques.le.roux Jacques Le Roux added a comment - Maybe unrelated but beware of OFBIZ-6275
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Adrian,

        I have attached a new patch that would take down the warnings to about 194. I appreciate if you can look into it and apply. The remaining warnings seem to be harder because most of them are about deprecated and external APIs. So I think I will leave those for the experts and close this issue after you apply the patch.

        Show
        taher Taher Alkhateeb added a comment - Hi Adrian, I have attached a new patch that would take down the warnings to about 194. I appreciate if you can look into it and apply. The remaining warnings seem to be harder because most of them are about deprecated and external APIs. So I think I will leave those for the experts and close this issue after you apply the patch.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Second patch committed in rev 1674064.

        Taher,

        Some parts of your patch would not apply, so I skipped those files.

        Also, you removed a finally block from DatabaseUtil.java, so I skipped that file. Removing a finally block is not an acceptable means to fix a compiler warning.

        There was one change that included a tab character. Be careful to use only spaces for indentation. I replaced the tab character with spaces.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Second patch committed in rev 1674064. Taher, Some parts of your patch would not apply, so I skipped those files. Also, you removed a finally block from DatabaseUtil.java, so I skipped that file. Removing a finally block is not an acceptable means to fix a compiler warning. There was one change that included a tab character. Be careful to use only spaces for indentation. I replaced the tab character with spaces.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Taher,

        1. What is the strategy on applying patches?
        2. What is the normal expected number of days for a commit to enter the system?
        3. How should we setup our expectations and / or reminders accordingly?
        1. There are no established rules. Personaly I 1st apply patches on bug fixes by order of importance, then improvements and then new features. New features are last because they often need longer reviews.
        2. It really depends, some patches are only applied years after, still they are. This said I sometimes update patches to avoid losing them for ever...
        3. Again no rules, when I feel something is important (eg really warnings are not that important even if I agree annoying) I don't hesitate to remind someone after a week and then a month, etc. But you can't expect committers (remember: volunteers on free time) to jump when you ask them
        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Taher, What is the strategy on applying patches? What is the normal expected number of days for a commit to enter the system? How should we setup our expectations and / or reminders accordingly? There are no established rules. Personaly I 1st apply patches on bug fixes by order of importance, then improvements and then new features. New features are last because they often need longer reviews. It really depends, some patches are only applied years after, still they are. This said I sometimes update patches to avoid losing them for ever... Again no rules, when I feel something is important (eg really warnings are not that important even if I agree annoying) I don't hesitate to remind someone after a week and then a month, etc. But you can't expect committers (remember: volunteers on free time) to jump when you ask them
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        I will look at this after ApacheCon.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - I will look at this after ApacheCon.
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Jacques,

        It seems interesting that my patch is now eight days old and fixing over a hundred warnings in trunk and it is not yet applied. What is the strategy on applying patches? What is the normal expected number of days for a commit to enter the system? How should we setup our expectations and / or reminders accordingly?

        I feel both confused and disappointed for not being able to work on the system easily or quickly. If commits are slow to incorporate to trunk which are trivial, I wonder if I can ever push anything significant.

        Show
        taher Taher Alkhateeb added a comment - Hi Jacques, It seems interesting that my patch is now eight days old and fixing over a hundred warnings in trunk and it is not yet applied. What is the strategy on applying patches? What is the normal expected number of days for a commit to enter the system? How should we setup our expectations and / or reminders accordingly? I feel both confused and disappointed for not being able to work on the system easily or quickly. If commits are slow to incorporate to trunk which are trivial, I wonder if I can ever push anything significant.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hè Taher, I think Adrian got it, we (other committers) did as well .

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hè Taher, I think Adrian got it, we (other committers) did as well .
        Hide
        taher Taher Alkhateeb added a comment -

        Bump, patch is waiting!

        Show
        taher Taher Alkhateeb added a comment - Bump, patch is waiting!
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Adrian, I appreciate your support in applying the patch.

        Show
        taher Taher Alkhateeb added a comment - Hi Adrian, I appreciate your support in applying the patch.
        Hide
        taher Taher Alkhateeb added a comment -

        and a bump! patch is ready and cleaned up

        Show
        taher Taher Alkhateeb added a comment - and a bump! patch is ready and cleaned up
        Hide
        taher Taher Alkhateeb added a comment -

        bump. Please apply the corrected patch

        Show
        taher Taher Alkhateeb added a comment - bump. Please apply the corrected patch
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Adrian,

        Updated patch is available

        Show
        taher Taher Alkhateeb added a comment - Hi Adrian, Updated patch is available
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Please provide an updated patch.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Please provide an updated patch.
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Adrian,

        Good catch. The reason I did not apply the generics is because I thought I would break the function signature of the callee object. Your solution looks good. However, if you look at OfbizBshBsfEngine.java for example, you will see that I will actually break the signature so I cannot do anything about it.

        Do you want me to repatch or can you correct it from your side?

        Show
        taher Taher Alkhateeb added a comment - Hi Adrian, Good catch. The reason I did not apply the generics is because I thought I would break the function signature of the callee object. Your solution looks good. However, if you look at OfbizBshBsfEngine.java for example, you will see that I will actually break the signature so I cannot do anything about it. Do you want me to repatch or can you correct it from your side?
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment - - edited

        Taher,

        Thank you for working on this! One thing has me confused: You make use of the UtilGenerics methods in some places, but not in others - where you add a @SuppressWarnings annotation instead. Why not always use UtilGenerics?

        Example from your patch:

            @SuppressWarnings("rawtypes")
            public void testJSONToMap() throws Exception {
                Converter<JSON, Map> converter = Converters.getConverter(JSON.class, Map.class);
                Map<String,String> map, convertedMap;
                map = new HashMap<String,String>();
                map.put("field1", "value1");
                JSON json = JSON.from(map);
                convertedMap = UtilGenerics.toMap(converter.convert(json));
                assertEquals("JSON to Map", map, convertedMap);
            }
        

        The @SuppressWarnings annotation is not needed:

            public void testJSONToMap() throws Exception {
                Converter<JSON, Map<String,String>> converter = UtilGenerics.cast(Converters.getConverter(JSON.class, Map.class));
                Map<String,String> map, convertedMap;
                map = new HashMap<String,String>();
                map.put("field1", "value1");
                JSON json = JSON.from(map);
                convertedMap = UtilGenerics.toMap(converter.convert(json));
                assertEquals("JSON to Map", map, convertedMap);
            }
        
        Show
        adrianc@hlmksw.com Adrian Crum added a comment - - edited Taher, Thank you for working on this! One thing has me confused: You make use of the UtilGenerics methods in some places, but not in others - where you add a @SuppressWarnings annotation instead. Why not always use UtilGenerics? Example from your patch: @SuppressWarnings( "rawtypes" ) public void testJSONToMap() throws Exception { Converter<JSON, Map> converter = Converters.getConverter(JSON.class, Map.class); Map< String , String > map, convertedMap; map = new HashMap< String , String >(); map.put( "field1" , "value1" ); JSON json = JSON.from(map); convertedMap = UtilGenerics.toMap(converter.convert(json)); assertEquals( "JSON to Map" , map, convertedMap); } The @SuppressWarnings annotation is not needed: public void testJSONToMap() throws Exception { Converter<JSON, Map< String , String >> converter = UtilGenerics. cast (Converters.getConverter(JSON.class, Map.class)); Map< String , String > map, convertedMap; map = new HashMap< String , String >(); map.put( "field1" , "value1" ); JSON json = JSON.from(map); convertedMap = UtilGenerics.toMap(converter.convert(json)); assertEquals( "JSON to Map" , map, convertedMap); }
        Hide
        taher Taher Alkhateeb added a comment - - edited

        I have created a new attachment called warnings_patch_2.patch and deleted the other patch to avoid confusion. This will bring down the number of warnings to 294. The patch includes the followings

        • removal of deprecated code causing some warnings
        • Introduction of generics to many collections
        • conversion of many maps and lists to java API instead of the old fastmap and fastlist from javolution
        • removal of redundant and unnecessary suppress tags
        • addition of suppress tags to places where the issue is not solvable
        • cleaning up iterator code to use generics
        • refactoring some code where the generics are incorrectly applied

        Please apply

        Show
        taher Taher Alkhateeb added a comment - - edited I have created a new attachment called warnings_patch_2.patch and deleted the other patch to avoid confusion. This will bring down the number of warnings to 294. The patch includes the followings removal of deprecated code causing some warnings Introduction of generics to many collections conversion of many maps and lists to java API instead of the old fastmap and fastlist from javolution removal of redundant and unnecessary suppress tags addition of suppress tags to places where the issue is not solvable cleaning up iterator code to use generics refactoring some code where the generics are incorrectly applied Please apply
        Hide
        taher Taher Alkhateeb added a comment -

        bump! please apply the second patch

        Show
        taher Taher Alkhateeb added a comment - bump! please apply the second patch
        Hide
        taher Taher Alkhateeb added a comment -

        Please apply the attached patch. It removes redundant suppresses and fixes void types

        Show
        taher Taher Alkhateeb added a comment - Please apply the attached patch. It removes redundant suppresses and fixes void types
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Fixed in rev 1669537. Thanks!

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Fixed in rev 1669537. Thanks!
        Hide
        taher Taher Alkhateeb added a comment -

        Please apply the attached patch which eliminates all unused imports

        Show
        taher Taher Alkhateeb added a comment - Please apply the attached patch which eliminates all unused imports

          People

          • Assignee:
            adrianc@hlmksw.com Adrian Crum
            Reporter:
            taher Taher Alkhateeb
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development