Skip to content

Conversation

@hwielenberg
Copy link

No description provided.

@hwielenberg hwielenberg requested a review from lukasoyen June 7, 2017 21:52
Copy link
Member

@lukasoyen lukasoyen left a comment

Choose a reason for hiding this comment

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

Looks good!

  • License headers
  • Nice code - everyThingIsAwesome
  • Tests

Could you write a few words on why Windows is no longer supported and bump the JDK version in .travis-ci.yml to make Travis happy?

If Travis is happy, I am happy :)

@@ -0,0 +1,30 @@
#Dont use maven directly, because all maven dependencies downloaded during build time won't be in the image
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a dockerfile? I guess it's fine.

You can also install Graphviz to generate class diagrams. This is optional.

choco install graphviz
Currently Windows is not supported.
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand on why Windows is not supported anymore?

Copy link
Author

Choose a reason for hiding this comment

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

There were a few reasons.
One that i can remember is reading from command line produces different line endings. So the unit tests fail on windows.

<compilerSource>1.7</compilerSource>
<compilerCompliance>1.7</compilerCompliance>
<compilerTargetPlatform>1.7</compilerTargetPlatform>
<compilerSource>1.8</compilerSource>
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a JDK version bump in the .travis-ci.yml aswell, to make Travis happy.

}
}

if(everyThingIsAwesome) {
Copy link
Member

Choose a reason for hiding this comment

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

everyThingIsAwesome @chndt 👍

for (VariableDeclaration variableDeclaration : parameters) {
visitor.visitDoubleDispatched(variableDeclaration);
}
// if(wrapperFunctionObjectDeclaration != null){
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary code is unnecessary (happens a few times)? Yet again, I don't really care.

@lukasoyen
Copy link
Member

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