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

Eclipse 4.8.0 (JDT) #262

Merged
merged 6 commits into from
Jul 19, 2018
Merged

Eclipse 4.8.0 (JDT) #262

merged 6 commits into from
Jul 19, 2018

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Jul 19, 2018

Made Spotless-Eclipse-JDT 4.8.0 default version.
Added functionality to lookup Maven Coordinate information from Jar-State as @nedtwigg proposed in #234.
This PR fixes issues #226 and #191.

@fvgh fvgh requested a review from nedtwigg July 19, 2018 18:34
@fvgh
Copy link
Member Author

fvgh commented Jul 19, 2018

@nedtwigg Not so sure about the change record. Should all CHANGE.md files refer this PR? Should Spotless CHANGE.md also mention #226?

@nedtwigg
Copy link
Member

Should Spotless CHANGE.md also mention #226?

Your call.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just the tiniest of nitpicks.

private static Class<?> getClass(State state) {
if (state.getMavenCoordinate(MAVEN_GROUP_ARTIFACT).isPresent())
return state.loadClass(FORMATTER_CLASS);
return state.loadClass(FORMATTER_CLASS_OLD);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ifs without brackets are dangerous. I would change to this:

if ( ) {
   return
} else {
   return
}

@@ -42,8 +42,7 @@
public final class JarState implements Serializable {
private static final long serialVersionUID = 1L;

@SuppressWarnings("unused")
private final Set<String> mavenCoordinates;
private final TreeSet<String> mavenCoordinates;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I almost missed this one. There are corner-cases where classpath order matters. Unless we have a really compelling reason, I think we should keep this as Set and not TreeSet

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to emphasize that I expect a deterministic result for getMavenCoordinates for the same input. I do not insist. What's your final decision?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then feel free to press the merge button, and I'll test the master integration build on some of my projects

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, THX.

@fvgh fvgh merged commit 420d688 into master Jul 19, 2018
@fvgh fvgh deleted the eclipse_4_8_0_jdt branch July 20, 2018 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants