[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15859416#comment-15859416 ] ASF GitHub Bot commented on FELIX-4848: --- Github user cschneider closed the pull request at: https://github.com/apache/felix/pull/18 > Split ResolverImpl > -- > > Key: FELIX-4848 > URL: https://issues.apache.org/jira/browse/FELIX-4848 > Project: Felix > Issue Type: Improvement > Components: Resolver >Affects Versions: resolver-1.0.0 >Reporter: Christian Schneider > Attachments: Dependencies in resolver package after patch.png, > FELIX-4848-1.patch > > > ResolverImpl currently contains about 2300 lines of code. That is way too big > for a single class. > I looked into it and found that the checkDynamicPackageSpaceConsistency and > checkPackageSpaceConsistency methods and their dependent methods form a nice > subset. I would move that into a class ConsistencyCheck. > Both would share all of the inner classes of ResolverImpl but nothing else. > So I think i would make sense to move these inner classes to separate files. > These changes should nicely split the classes into > ResolverImpl : 1400 lines > ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15278862#comment-15278862 ] Thomas Watson commented on FELIX-4848: -- Sorry I have not taken a closer look at this contribution. I suspect it is now stale and cannot be applied easily. It also is likely unreasonable to ask you to revisit this given how unresponsive we have been in getting this work applied. On a more cheerful note. I tried to do my own bit of clean up on the code in FELIX-5251. Christian, not sure if you would be willing to have a look at the current code in trunk to see if it has improved enough to make this defect obsolete. > Split ResolverImpl > -- > > Key: FELIX-4848 > URL: https://issues.apache.org/jira/browse/FELIX-4848 > Project: Felix > Issue Type: Improvement > Components: Resolver >Affects Versions: resolver-1.0.0 >Reporter: Christian Schneider > Fix For: resolver-2.0.0 > > Attachments: Dependencies in resolver package after patch.png, > FELIX-4848-1.patch > > > ResolverImpl currently contains about 2300 lines of code. That is way too big > for a single class. > I looked into it and found that the checkDynamicPackageSpaceConsistency and > checkPackageSpaceConsistency methods and their dependent methods form a nice > subset. I would move that into a class ConsistencyCheck. > Both would share all of the inner classes of ResolverImpl but nothing else. > So I think i would make sense to move these inner classes to separate files. > These changes should nicely split the classes into > ResolverImpl : 1400 lines > ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14576793#comment-14576793 ] Christian Schneider commented on FELIX-4848: [~tjwatson] Hi Thomas. Many thanks for your offer to do a review. It took a bit longer as I had problems with the maven build. I created a pull request now. I mainly extracted two parts out of ResolverImpl: ConsistencyChecker: Contains all checks from ResolverImpl. It accesses the inner classes that form the model of ResolverImpl but nothing else from it CapabilityFinder: Encapsulates finding capabilities for packages and the caching that is done to speed this up I would like to improve the printing of the error messages too but defered this to a later issue to make this a simpler refactoring that mainly moves code. One downside is that we now have a small circular usage of the inner classes of ResolverImpl. I could solve this by extracting the inner classes if you want. In that case though I would propose to create a new package for ResolverImpl, ConsistencyChecker, CapabilityFinder and the inner classes so it is clear that they are only to be used in this context. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.4.0 Attachments: Dependencies in resolver package after patch.png, FELIX-4848-1.patch ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14576775#comment-14576775 ] ASF GitHub Bot commented on FELIX-4848: --- GitHub user cschneider opened a pull request: https://github.com/apache/felix/pull/18 FELIX-4848 : Split ResolverImpl You can merge this pull request into a Git repository by running: $ git pull https://github.com/cschneider/felix FELIX-4848 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/felix/pull/18.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #18 commit e3024b0ffe74fb9e343ed4ae3224172223a92613 Author: Christian Schneider ch...@die-schneider.net Date: 2015-06-02T14:22:43Z FELIX-4848 : Split ResolverImpl Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.4.0 Attachments: Dependencies in resolver package after patch.png, FELIX-4848-1.patch ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14564417#comment-14564417 ] Christian Schneider commented on FELIX-4848: [~rickhall] Hi Richard. As most of the code in ResolverImpl is from you I suggest you decide about this patch. If you do not intend to apply it I suggest we close this issue. I do not like having jira issues dangling around for too long. Especially refactoring patches typically are not applicable after some time anyway. If you change your mind at any time I am willing to create a new issue and patch. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.4.0 Attachments: Dependencies in resolver package after patch.png, FELIX-4848-1.patch ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14564724#comment-14564724 ] Thomas Watson commented on FELIX-4848: -- The latest patch is incomplete, it is missing the ConsistencyCheck class. I don't have a strong opinion on the need for this patch, but if it helps others understand the code better then it seems like it could be a good thing. Not seeing the complete patch I do have some concerns. Did you really just move code from one class to another without making functional changes? Or did you fix some bugs along the way? I ask because the changes to the Util class may be problematic. For getSymbolicName and getVersion you now use the osgi.identity namespace to lookup the capability with the info. That is good, but at one point the WrappedResource did not pay attention to the namespace param (see FELIX-4727). I'm not saying the code should not use the namespace here, but it does require that the input resources pay attention to the namespace (we certainly do in Equinox, but I don't know about other users). But the other issue is that the new impls of these methods will throw out of bounds exceptions if the resource has no osgi.identity capabilities (e.g. for R3 bundles). Perhaps not important to most, but Equinox still supports R3 bundles and I am pretty sure Felix does also. Once the complete patch is posted it is going to be hard to confirm that all that was done is a refactoring of the code, without changing the functionality and I worry about that based on the changes done to the Util class. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.4.0 Attachments: Dependencies in resolver package after patch.png, FELIX-4848-1.patch ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14564736#comment-14564736 ] Christian Schneider commented on FELIX-4848: If you are positive about the refactoring then I will create a new patch and make sure I do not do any functional changes. I am not sure anymore about the current patch. I remember I either did or wanted to do some improvements for the error reporting but these should probably better be done separately. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.4.0 Attachments: Dependencies in resolver package after patch.png, FELIX-4848-1.patch ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14565276#comment-14565276 ] Thomas Watson commented on FELIX-4848: -- If you go through the effort to create a new patch that has no intended functional changes then I think an effort should be made to review and get the change merged into trunk. I am willing to review and run the changes through my equinox tests, but this will take a little bit of time on my side to get to (weeks, not months). In the end we then need to convince a committer to release it. We should be able to get someone to do that easily enough after I review and test. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.4.0 Attachments: Dependencies in resolver package after patch.png, FELIX-4848-1.patch ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14488112#comment-14488112 ] Christian Schneider commented on FELIX-4848: I did not mention any rule but 2300 lines is far beyond any limits I have seen recently. Of course you are right that simply splitting a class in half would not work if then the two classes are highly entangled. I checked the dependencies before proposing the split. ResolverImpl would have 3 calls to ConsistencyCheck and there would be no calls in the other direction. So it would be possible to understand ConsistencyCheck individually. Besides that the scope is also nicely described by the class name. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.2.0 ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (FELIX-4848) Split ResolverImpl
[ https://issues.apache.org/jira/browse/FELIX-4848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14487790#comment-14487790 ] Richard S. Hall commented on FELIX-4848: I am always humored by such rules (e.g., a file shouldn't be over X many lines, etc.)...as always, it just depends. I don't see how distributing complex code over multiple files makes it any more tractable for someone than having those same details in a single file. Just an observation. Split ResolverImpl -- Key: FELIX-4848 URL: https://issues.apache.org/jira/browse/FELIX-4848 Project: Felix Issue Type: Improvement Components: Resolver Affects Versions: resolver-1.0.0 Reporter: Christian Schneider Fix For: resolver-1.2.0 ResolverImpl currently contains about 2300 lines of code. That is way too big for a single class. I looked into it and found that the checkDynamicPackageSpaceConsistency and checkPackageSpaceConsistency methods and their dependent methods form a nice subset. I would move that into a class ConsistencyCheck. Both would share all of the inner classes of ResolverImpl but nothing else. So I think i would make sense to move these inner classes to separate files. These changes should nicely split the classes into ResolverImpl : 1400 lines ConsistencyCheck : 600 lines -- This message was sent by Atlassian JIRA (v6.3.4#6332)
