-
Notifications
You must be signed in to change notification settings - Fork 682
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
Update Play source for build compability with JDK9+ #1252
Merged
+85
−87
Merged
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
78b7141
update Play for build with jdk9+. Update libs
tomparle 0908d58
merged upstream branch
tomparle b28fa4a
fix previous loading class method using _ character now reserved in j…
tomparle 1421f5c
fix selenium test
tomparle 55af1a5
update java source detection mechanism to use the version specified i…
tomparle c1612c6
forgot to add version 9
tomparle 830cda3
fix failing tests due to upgrade to JDK9. Upgrade test libraries and …
tomparle a819900
ugly fix to make test pass with annotation parameter values with or w…
tomparle 056285d
set java source and target version to use for the build, and remove u…
tomparle b6cf1f0
revert back method to static. Fix calls to deprecated '_' method, rep…
tomparle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you made it static? It's private method, it cannot be used from outside. Please make it non-static back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback and having taken the time to test for multiple versions !
I made this method static because my editor automatically notifies me when a method could be declared static (as an utility method), which I did here because it does not use any instance field or method.
No problem if you still prefer to make it non-static back !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomparle Ok, I know that editor notification. I also used it many years ago.
Now I am pretty sure that
static
is a bad practice in most cases. For many reasons. The fact that the method doesn't use any fields is not the key factor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asolntsev thanks for the feedback. I removed the static identifier from the method. I'd also be glad if you could share me some reasons of why it can be a bad practice, or give me a link about it !
Also, I just found out a regression that I did not see before, in the
#{form}
tag of the CRUD module (this module is separated from the framework, which is why I did not see it at first). This has been fixed.I think it would also be neat to add the following kind of migration note about the removed method "_" in Groovy templates - even if I do not think it has been much used anywhere except from the framework itself (how do we add this to the future release notes ?) :
The shorthand method
_(String className)
to load a class in Groovy templates has been removed since the underscore is now a reserved keyword in JDK9 and later versions. Please use the method__loadClass(String className)
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomparle will this restriction of JDK9 on the Groovy templates have a knock on effect on https://github.com/codeborne/play-fastergt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tazmaniax indeed, I proposed some changes to the
form
andtypes
tag implementation, see codeborne/play-fastergt#1As I do not use this tag implementation, I wish someone who does could test it in his app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomparle I'll test shortly, thx! Out of interest is there a reason why you don't use this rendering implementation as it would appear to be an improvement over the standard implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tazmaniax First, thank you for make me discover this implementation I was not aware of. I guess it's the main raison I was not using it ! But if indeed this is an improvement I will try it for sure.
Another issue is that I have already customized heavily CRUD tags to support additional features, and I guess I would have to merge it with faster GT implementation which would require some work. Anyway, please let me know of the test results !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also done some customisation of the CRUD tags and it would be interesting to maybe coalesce them into this project if they would be of value to others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea ! I will see that after I test to integrate them on my project, and tell you if I can add some features useful to others.