Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add (diamond) type arguments to lots of files, clean up unused imports #933

Closed
wants to merge 4 commits into from

Conversation

bencomp
Copy link
Contributor

@bencomp bencomp commented Sep 23, 2014

(This pull request is an improved 'version' of #931.)

Where raw types were used, most calls have been typed with either the full class name or the diamond operator provided by Java 7. In some cases, Class references were changed to Class<?>. I manually removed class casts where the type additions made them obviously redundant. In DataversePage.getContents() I couldn't add type arguments because the List used internally contains different types and addAll() does not work well with <? extends DvObject> (hints can be found in Java generics and the addAll method.

CommandHelper and ServiceRegistry need another closer look, as my changes may touch upon the purpose or design of these classes.

I let Eclipse remove unused imports, which accounts for a large part of the deletions.

In a few places I corrected a typo or two.

Raw `List`, `ArrayList`, `Class` and other types were used in many files. I used Eclipse and NetBeans to identify and transform these into fully typed variables/parameters/arguments and diamond-typed definitions and references.

In a few places this was not possible; I added a FIXME in one of them (in DataversePage.java).
@pdurbin
Copy link
Member

pdurbin commented Sep 23, 2014

@bencomp as I mentioned at http://irclog.iq.harvard.edu/dataverse/2014-09-23#i_12869 it would be very helpful if you would provide us with the warning you are trying to correct. Please point me to a line in one of the files and the warning you are seeing either from Netbeans or a command line tool. Thanks!

@pdurbin pdurbin self-assigned this Sep 23, 2014
@pdurbin pdurbin added the Type: Suggestion an idea label Sep 23, 2014
@bencomp
Copy link
Contributor Author

bencomp commented Sep 23, 2014

In general, this is called the "unchecked assignment" warning, explained at http://www.angelikalanger.com/GenericsFAQ/FAQSections/ParameterizedTypes.html#FAQ202. Using raw types causes these warnings, because the compiler cannot know at compile time that casts from items in a e.g. raw List or Map are indeed of the type that is assigned to them.
If you pass -Xlint:unchecked to the java compiler as an argument, it will output these warnings during compilation.

IntelliJIDEA finds 170 occurrences of this type of warning in the master branch.
I can't find an option in Checkstyle to get such warnings in reports (available as Jenkins plugin).

In NetBeans you can see the compiler warnings by enabling Raw Types under Standard Javac Warnings on the Hints tab in the Editor options.

In the screenshot NetBeans showing a warning, NetBeans shows a warning for AdvancedSearchPage.java on line 30:

[unchecked] unchecked conversion
required: Map<Long, List<DatasetFieldType>>
found: HashMap

@pdurbin
Copy link
Member

pdurbin commented Sep 24, 2014

As of a7d24b7 I can run this from the command line to see the details of all the warnings:

mvn -DcompilerArgument=-Xlint:unchecked compile

@michbarsinai what do you think about all of this? You're our type safety guy. :)

public static final LruCache<Long,List<DataverseFacet>> cache = new LruCache();

gives this warning:

[WARNING] /Users/pdurbin/NetBeansProjects/dataverse/src/main/java/edu/harvard/iq/dataverse/DataverseFacetServiceBean.java:[20,69] unchecked conversion
required: edu.harvard.iq.dataverse.util.LruCache<java.lang.Long,java.util.List<edu.harvard.iq.dataverse.DataverseFacet>>
found: edu.harvard.iq.dataverse.util.LruCache

@michbarsinai
Copy link
Member

The compiler is correct in a very skewed way. Just add <> so that the 'new' call looks like new LruCache<>() and you'll be fine. I'd say more, but I'm typing this on a smartphone :-(

Sent from my iPhone

On 24 בספט׳ 2014, at 23:33, Philip Durbin notifications@github.com wrote:

As of a7d24b7 I can run this from the command line to see the details of all the warnings:

mvn -DcompilerArgument=-Xlint:unchecked compile

@michbarsinai what do you think about all of this? You're our type safety guy. :)

public static final LruCache<Long,List> cache = new LruCache();

gives this warning:

[WARNING] /Users/pdurbin/NetBeansProjects/dataverse/src/main/java/edu/harvard/iq/dataverse/DataverseFacetServiceBean.java:[20,69] unchecked conversion
required: edu.harvard.iq.dataverse.util.LruCache>
found: edu.harvard.iq.dataverse.util.LruCache


Reply to this email directly or view it on GitHub.

@pdurbin
Copy link
Member

pdurbin commented Jan 14, 2015

I'm seeing "We can’t automatically merge this pull request" which isn't surprising because 100 files were changed and the code is actively being worked on. This is too many files for us to reason about at once.

@bencomp I'm going to close this pull request but yes, we are certainly thinking about the ideas it represents, as you asked at http://irclog.iq.harvard.edu/dataverse/2015-01-14#i_15308 . Thanks!

It sounds like you aren't offended, which is great. :)

Let's put our energy toward #775 in which we are defining our coding standards in a Google doc for now, where public comments are enabled.

@pdurbin pdurbin closed this Jan 14, 2015
@pdurbin pdurbin removed their assignment Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants