Remove classes with no references #1453
Conversation
|
I built, ran the unit tests and ran the server with the simulator. Thanks for cleaning up! LGTM [INFO] ------------------------------------------------------------------------ |
|
Thank you @pdube! |
|
@GabrielBrascher I'm getting a merge error on this PR to master. can you have a look and rebase if needed? |
|
@DaanHoogland, @GabrielBrascher, @DaanHoogland, are you going to merge this PR? |
|
@rafaelweingartner yes, I will be running CI on this when the merge conflicts have been resolved. nothing should be committed to master without CI run against it IMO... |
|
@swill thanks |
|
haha, trying to be. it is turning into a full time job... |
rafaelweingartner
force-pushed
the
brascher-removeUnusedClasses2
branch
2 times, most recently
from
0666e0d
to
9b9560c
Compare
- org.apache.cloudstack.framework.jobs.JobCancellationException (removed variable JobCancellationException in com.cloud.utils.SerialVersionUID) - org.apache.cloudstack.ldap.NoSuchLdapUserException (removed test file /cloud-plugin-user-authenticator-ldap/test/groovy/org/apache/cloudstack/ldap/NoSuchLdapUserExceptionSpec.groovy) - com.cloud.agent.api.storage.CreateVolumeOVAAnswer - com.cloud.exception.MissingParameterValueException - org.apache.cloudstack.api.response.StatusResponse - org.apache.cloudstack.api.response.VolumeDetailResponse - org.apache.cloudstack.api.response.UpgradeVmResponse - org.apache.cloudstack.api.response.AddIpToVmNicResponse - org.apache.cloudstack.api.response.TemplateZoneResponse (at org.apache.cloudstack.api.response.TemplateResponse, there is this comment "To avoid breaking backwards compatibility, we still treat a template at different zones as different templates, so not embedding template_zone information in this TemplateZoneResponse set. `private Set<TemplateZoneResponse> zones;`" but right now it is not used) - org.apache.cloudstack.api.response.NicDetailResponse
rafaelweingartner
force-pushed
the
brascher-removeUnusedClasses2
branch
from
9b9560c
to
4179606
Compare
|
@swill , @DaanHoogland , |
|
@rafaelweingartner the merge errors occured during testing as merging is part of the integration tests. I agree with @swill about the test being needed for this change. thought I think it makes no sense/adds no value for some other changes. The latter being UI and added tests. Every thing else should undergo the regression suite. |
|
@DaanHoogland I understand your points on requiring functional tests and accept it. |
|
I am having other problems (after the merge) in my test environment, not related to the PR. |
CI RESULTSSummary of the problem(s): Associated Uploads
Uploads will be available until Comment created by |
|
The one problem that is showing up in my CI does not appear to be related to this PR, but this is the first time I have ever seen this error. If someone else can confirm that this would not be related, I would appreciate it. :) |
|
Can I get one more code review on this one??? Thx... |
|
LGTM @swill |
…asses2 Remove classes with no referencesI used UCDetector (http:https://www.ucdetector.org/) as a plugin for Eclipse. With this tool, I discovered a lot of code without any reference (variables, methods and classes). Following the work that was done at [#1448]; this pull request had the goal of removing some of these classes. To check if I wasn't missing anything I searched for any file that could reference some of those classes. As I haven't found any way of these classes being used, they were removed. Note that some of them I found other references, but references such as commented lines or tests, nothing that could indicate their use (as XML files configuring beans or another class instantiating an object with "new"). Waiting for tests. Please tell me if I am missing something. Removed Classes: - org.apache.cloudstack.framework.jobs.JobCancellationException (**Note:** removed variable JobCancellationException in com.cloud.utils.SerialVersionUID) - org.apache.cloudstack.ldap.NoSuchLdapUserException (**Note:** removed test file /cloud-plugin-user-authenticator-ldap/test/groovy/org/apache/cloudstack/ldap/NoSuchLdapUserExceptionSpec.groovy) - com.cloud.agent.api.storage.CreateVolumeOVAAnswer - com.cloud.exception.MissingParameterValueException - org.apache.cloudstack.api.response.StatusResponse - org.apache.cloudstack.api.response.VolumeDetailResponse - org.apache.cloudstack.api.response.UpgradeVmResponse - org.apache.cloudstack.api.response.AddIpToVmNicResponse - org.apache.cloudstack.api.response.TemplateZoneResponse (**Note:** at org.apache.cloudstack.api.response.TemplateResponse, there is this comment "To avoid breaking backwards compatibility, we still treat a template at different zones as different templates, so not embedding template_zone information in this TemplateZoneResponse set. `private Set<TemplateZoneResponse> zones;`" but right now it is not used) - org.apache.cloudstack.api.response.NicDetailResponse * pr/1453: Removed classes with no reference Signed-off-by: Will Stevens <[email protected]>
I used UCDetector (http:https://www.ucdetector.org/) as a plugin for Eclipse. With this tool, I discovered a lot of code without any reference (variables, methods and classes).
Following the work that was done at [https://github.com//pull/1448]; this pull request had the goal of removing some of these classes. To check if I wasn't missing anything I searched for any file that could reference some of those classes. As I haven't found any way of these classes being used, they were removed. Note that some of them I found other references, but references such as commented lines or tests, nothing that could indicate their use (as XML files configuring beans or another class instantiating an object with "new").
Waiting for tests. Please tell me if I am missing something.
Removed Classes:
variable JobCancellationException in com.cloud.utils.SerialVersionUID)
/cloud-plugin-user-authenticator-ldap/test/groovy/org/apache/cloudstack/ldap/NoSuchLdapUserExceptionSpec.groovy)
org.apache.cloudstack.api.response.TemplateResponse, there is this
comment "To avoid breaking backwards compatibility, we still treat a
template at different zones as different templates, so not embedding
template_zone information in this TemplateZoneResponse set.
private Set<TemplateZoneResponse> zones;" but right now it is not used)