Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix compiler warnings #83

Closed
wants to merge 1 commit into from

2 participants

@philwebb
Owner

Warnings in all src/main/java folders have been reviewed and fixed with the following exceptions:

  • changes that might break binary compatibility
  • changes due to reliance on external deprecated code
  • changes that seem contentious or may require additional review

In addition src/test/java folders have been reviewed and fixed up-to spring-jdbc. The use of
deprecated EasyMock calls have not yet been updated.

The eclipse 'organize imports' command has also been used to fix any unused import declarations and provide a consistent style.

@philwebb
Owner

Chris,

The supplied commit fixes the majority of the warnings in the spring code. Unfortunately I have not had time to fix all test projects as yet, I got as far as spring-jdbc. I have also not updated any gradle configuration as yet.

One aspect of the patch that concerns me a little is that some changes may require spring users to change their code (although the changes are binary compatible). I found this a little late, otherwise I would have submitted these as a separate patch.

For example,
someMethod(Class clazz) changes to someMethod(Class<?> clazz)

This code continues to compile, but
someMethod(List classes) changes to someMethod(List> classes)

This may stop existing code compiling, depending on how it was written:
List myList = new ArrayList();
SomeClass.someMethod(myList); //broken

but:
List myList = new ArrayList();
SomeClass.someMethod(myList); //fine for both old and new

There does not seem to be a easy solution for this. I personally think that the generics should probably be added.

Phil.

@cbeams

First, thanks!

I haven't read the above completely yet, but will. I've glanced over the commit and notice that there is a bit of trailing whitespace (at least one incidence) and many places in which newlines between imports have been removed. Eclipse does this. It will take the following:

org.junit...

org.springframework...

and turn it into

org.junit...
org.springframework...

It should remain the former.

@cbeams

Also, your rules around static imports are different than the convention in the framework. I would recommend simply leaving imports alone. This could be a little tricky to back out with such a large commit, but I'm sure it's possible. We'll figure it out. I can also show you what the conventions are for imports, but I cannot get Eclipse to strictly follow them; I do it by hand. So with this volume it's again probably best to leave them as-is.

@philwebb
Owner

I can fixup the whitespace with:

find . -type f -name ".java" -exec perl -p -i -e "s/[ \t]$//g" {} \;

Any suggestions for backing out the import changes?

@philwebb
Owner

I have restored the original imports and removed the whitespace. Do you want me to create a new pull request with these commits squashed?

It is a shame that STS "organize imports" cannot work, I find it generally useful. Have you considered changing the rules used in the framework to that STS formatting can be defined?

BTW during these changes I noticed that a lot of resource files are in /src/main/java as well as /src/main/resources. Is there a reason for this? I know that Maven would ignore non java files in /src/main/java, does gradle work differently?

@cbeams

Ok, thanks for doing that. I'm curious how you restored the imports. git reset -p would have been one way, but still pretty manual.

Basically we never rely strictly on formatters. I personally use 'organize imports', but always scrutinize the diff before committing and manually tweak as necessary to preserve existing formatting and conventions.

Do you mean there are cases in which the same resource file exists in both locations, or are you pointing out that we have a split between these two locations in general? Perhaps give a few examples and we'll discuss.

As far as the commits go, you can squash back into a single commit, put a commit comment back in place, and then force push back to this branch, at which point there will only be a single commit to consider merging.

By the way, in your original commit comment for acfbc64, line lengths went beyond the 72 character margin mentioned in the [contributor guidelines|https://github.com/SpringSource/spring-framework/wiki/Contributor-guidelines].

Also, given that this is such a substantive change, please create a JIRA issue (Task) and reference it with "Issue:" in the commit comment as mentioned in the guidelines.

Thanks!

@philwebb
Owner

To restore the imports I created a copy of the code repository in a different folder then hacked up something to copy the appropriate parts of the code from the correct location. I then manually fixed up the remaining errors.

Looking again I may have been mistaken about resources being duplicated. I hit ctrl-shift-R in eclipse and noticed the duplicated files but it turns out they are in /build/resources so I assume they are copied by gradle. I was not expecting to see resource files in src/test/java (for example /spring-beans/src/test/java/org/springframework/beans/factory/BeanFactoryUtilsTests-dependentBeans.xml), I assumed they would be in /src/test/resources (like /spring-beans/src/test/resources/org/springframework/beans/factory/xml/autowire-constructor-with-exclusion.xml). I wrongly assumed that at some point a migration to the maven folder structure happened and they ended up in there twice.

I will raise a JIRA, tidy up the log message and squash the commits before resubmitting.

Cheers.

@cbeams cbeams was assigned
@philwebb philwebb Fix compiler warnings
Warnings in all src/main/java folders have been reviewed and fixed
with the following exceptions:
 - changes that might break binary compatibility
 - changes due to reliance on external deprecated code
 - changes that seem contentious or may require additional review

In addition src/test/java folders have been reviewed and fixed
up-to spring-jdbc.  The use of deprecated EasyMock calls have not
yet been updated.

Issues: SPR-9431
15df1d1
@cbeams

Closing this PR, as I think it's no longer relevant (correct me if I'm wrong). We've done quite a bit of work on warnings, and there will probably be more to go, but it'll continue to be incremental.

@philwebb philwebb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 18, 2012
  1. @philwebb

    Fix compiler warnings

    philwebb authored
    Warnings in all src/main/java folders have been reviewed and fixed
    with the following exceptions:
     - changes that might break binary compatibility
     - changes due to reliance on external deprecated code
     - changes that seem contentious or may require additional review
    
    In addition src/test/java folders have been reviewed and fixed
    up-to spring-jdbc.  The use of deprecated EasyMock calls have not
    yet been updated.
    
    Issues: SPR-9431
Something went wrong with that request. Please try again.