Commons Discovery
  1. Commons Discovery
  2. DISCOVERY-15

implementing a commons-logging wrapper is an overkill

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5
    • Fix Version/s: 0.5
    • Labels:
      None

      Description

      org.apache.commons.discovery.log is a commons-logging implementation wrapper, moreover the 90% of classes that use a Log instance, have a static setter method.
      It should be enough just using commons-logging in the 'traditional' way

        Activity

        Simone Tripodi created issue -
        Hide
        Sebb added a comment -

        According to the SVN history, these classes were added because:

        "Added logging support. Copied 2 files from commons-logging (Log & LogFactory)
        to prevent circular dependencies during build. Support for a bootstrap mechanism
        to allow the default (VERY simple logging) to be replaced with app/system loggers."

        Not entirely sure where the circular dependency comes from, unless commons logging once depended on discovery?

        Show
        Sebb added a comment - According to the SVN history, these classes were added because: "Added logging support. Copied 2 files from commons-logging (Log & LogFactory) to prevent circular dependencies during build. Support for a bootstrap mechanism to allow the default (VERY simple logging) to be replaced with app/system loggers." Not entirely sure where the circular dependency comes from, unless commons logging once depended on discovery?
        Hide
        Simone Tripodi added a comment -

        Ah, yes, It could be that, for old commons-logging releases, they used the discovery to discover Log implementations
        BTW upgrading to latest commons-logging release (already done) the circular dependency is no more detected, is it fine for you removing the 'log' package from discovery?

        Moreover I'd like to remove also the below methods from classes...

        public static void setLog(Log _log) {
            log = _log;
        }
        
        Show
        Simone Tripodi added a comment - Ah, yes, It could be that, for old commons-logging releases, they used the discovery to discover Log implementations BTW upgrading to latest commons-logging release (already done) the circular dependency is no more detected, is it fine for you removing the 'log' package from discovery? Moreover I'd like to remove also the below methods from classes... public static void setLog(Log _log) { log = _log; }
        Hide
        Sebb added a comment -

        It does look as though the log classes are no longer needed.

        However, if you remove the methods, this will break binary compatibility.

        I think it would be safer to make at least one binary compatible release, which means deprecating rather than removing the methods.

        Show
        Sebb added a comment - It does look as though the log classes are no longer needed. However, if you remove the methods, this will break binary compatibility. I think it would be safer to make at least one binary compatible release, which means deprecating rather than removing the methods.
        Hide
        Simone Tripodi added a comment -

        Agreed, let's just remove the custom implementation, and maintain the binary compatibility.
        Thanks!

        Show
        Simone Tripodi added a comment - Agreed, let's just remove the custom implementation, and maintain the binary compatibility. Thanks!
        Hide
        Simone Tripodi added a comment -

        fixed on trunk, see r1088970

        Show
        Simone Tripodi added a comment - fixed on trunk, see r1088970
        Simone Tripodi made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Jochen Wiedmann added a comment -

        Would you please be so kind and explain how you can "remove the custom implementation" (aka remove classes, aren't you?) and "maintain the binary compatibility" at the same time?

        Show
        Jochen Wiedmann added a comment - Would you please be so kind and explain how you can "remove the custom implementation" (aka remove classes, aren't you?) and "maintain the binary compatibility" at the same time?
        Hide
        Simone Tripodi added a comment -

        Sure, Jochen, I realize that maybe the problem is not well explained.

        Situation was that we had a org.apache.commons.discovery.log package where there was a simple org.apache.commons.logging.Log implementation and related factory, and in discovery classes there are a lot of methods like:

            public static void setLog(org.apache.commons.logging.Log _log) {
                log = _log;
            }
        

        So, just removing the org.apache.commons.discovery.log package, and using the commons-logging in the traditional way, we removed the custom implementation (yes, remove classes) but maintained the binary compatibility since setters have not be touched

        Show
        Simone Tripodi added a comment - Sure, Jochen, I realize that maybe the problem is not well explained. Situation was that we had a org.apache.commons.discovery.log package where there was a simple org.apache.commons.logging.Log implementation and related factory, and in discovery classes there are a lot of methods like: public static void setLog(org.apache.commons.logging.Log _log) { log = _log; } So, just removing the org.apache.commons.discovery.log package, and using the commons-logging in the traditional way, we removed the custom implementation (yes, remove classes) but maintained the binary compatibility since setters have not be touched
        Hide
        Jochen Wiedmann added a comment -

        These classes are part of the release 0.4. They are public, hence part of the public API. Removing them breaks binary compatibility.

        Show
        Jochen Wiedmann added a comment - These classes are part of the release 0.4. They are public, hence part of the public API. Removing them breaks binary compatibility.
        Hide
        Simone Tripodi added a comment -

        Sure, but they were implemented just for common internal use, I didn't see any "damage" for clients.
        Shall I put back and just deprecate? Would it work better in therms of 0.5 release?
        Thanks in advance!

        Show
        Simone Tripodi added a comment - Sure, but they were implemented just for common internal use, I didn't see any "damage" for clients. Shall I put back and just deprecate? Would it work better in therms of 0.5 release? Thanks in advance!
        Hide
        Jochen Wiedmann added a comment -

        Noone knows, for example, that these haven't been subclassed and used. We should not speculate, whether this is the case or not. Please put back and deprecate.

        Show
        Jochen Wiedmann added a comment - Noone knows, for example, that these haven't been subclassed and used. We should not speculate, whether this is the case or not. Please put back and deprecate.
        Hide
        Simone Tripodi added a comment -

        Done, thanks for reviewing

        Show
        Simone Tripodi added a comment - Done, thanks for reviewing
        Hide
        Simone Tripodi added a comment -

        included in discovery-0.5 release

        Show
        Simone Tripodi added a comment - included in discovery-0.5 release
        Simone Tripodi made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            Simone Tripodi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development