Support for Java8 parameters #1084
Conversation
framework/src/play/utils/Java.java
Outdated
| // No import, so Java 7 will not break trying to load this class | ||
| java.lang.reflect.Parameter[] parameters = method.getParameters(); | ||
| String[] names = new String[parameters.length]; | ||
| for (int i = 0; i < parameters.length; i++) names[i] = parameters[i].getName(); |
There was a problem hiding this comment.
Don't use inline delclaration for for
|
This PR actually works with Java 7.
If you mean that in 1.5.x we can drop Java 7 support, then this PR can be
simplified.
…On Tue, 10 Jan 2017, 08:03 Alexandre Chatiron, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Please squash your change in one commit.
We will create a new branch 1.5.x and to merge this PR in.
Seems like the test is also failing on 1.4.x branch. I will check it
------------------------------
In framework/src/play/utils/Java.java
<#1084 (review)>
:
> @@ -194,10 +194,15 @@ public static Object invokeStatic(Method method, Object[] args) throws Exception
* Retrieve parameter names of a method
*/
public static String[] parameterNames(Method method) throws Exception {
- try {
- return (String[]) method.getDeclaringClass().getDeclaredField("$" + method.getName() + LocalVariablesNamesTracer.computeMethodHash(method.getParameterTypes())).get(null);
- } catch (Exception e) {
- throw new UnexpectedException("Cannot read parameter names for " + method, e);
+ if (Play.classes.java8) {
+ // No import, so Java 7 will not break trying to load this class
+ java.lang.reflect.Parameter[] parameters = method.getParameters();
+ String[] names = new String[parameters.length];
+ for (int i = 0; i < parameters.length; i++) names[i] = parameters[i].getName();
Don't use inline delclaration for for
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1084 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABmoKbUkIJatvulDKfZDMZvlkBmYZhe1ks5rQx8ugaJpZM4LeYgX>
.
|
|
Sorry my bad. so we will merge in 1.4.x after remarks fixes |
cbxp
force-pushed
the
java8-parameters
branch
from
5c5b4e5 to
812412f
Compare
|
I have fixed the for loop as was requested, and also squashed some commits, but not all. Squashing many changes with their own reasoning (commit messages) into a single big commit usually a not good idea, as it is quite hard or impossible to understand later the reasoning behind particular changes. Hopefully these 3 remaining commits are ok. |
|
The build seems to be failing in 1.4.x already... |
Indeed, see here for more details (solution proposed by @asolntsev): |
| @@ -1,7 +1,7 @@ | |||
| sudo: required | |||
| language: java | |||
| jdk: | |||
| - openjdk7 | |||
There was a problem hiding this comment.
Could you keep openjdk7 to be sure 1.4.x still compile with 1.7?
There was a problem hiding this comment.
Then we cannot build play... It would be nice to build with Java 8, but run with Java 7, but I don't know how to do that with travis
|
@xael-fry @angryziber @cbxp it's possible jdk:
- oraclejdk8
- openjdk7
addons:
apt:
packages:
- oracle-java8-installer
script:
- jdk_switcher use oraclejdk8
- # do stuff with Java 8
- jdk_switcher use oraclejdk7
- # do stuff with Java 7See for more details: https://docs.travis-ci.com/user/languages/java/ |
|
But after thinking about it, it we cannot compile it with JDK 7 we will not merge it in 1.4.x. but in a new branch 1.5.x. |
angryziber
force-pushed
the
java8-parameters
branch
from
b194dd5 to
9b13050
Compare
now 1.8 is default value
|
Dropped Java 7 support - now this can be merged to the newly created master branch for the upcoming 1.5.x release series |
And read java8 parameter names first before falling back to ehnhancer-generated fields for older Java.
… 7 is not supported by Play 1.4.x anyway
…ameter names fields. This class can be dropped when we drop Java 7 support. Plain LocalvariablesNamesEnhancer will not do it anymore, because parameter names are available using standard API in Java 8. Build of Play now requires Java 8, but will still run with Java 7.
…thod parameter names and considerably reduce enhancing
angryziber
force-pushed
the
java8-parameters
branch
from
9b13050 to
8797111
Compare
|
merged to |
|
All travis tests of this PR fails, reset master |
|
Sorry, originally it specified oraclejdk8, which was correct for Travis.
Later it was changed to openjdk8, which is not supported.
Andrei, can you please change it to oraclejdk8 in .travis.yml?
|
|
@xael-fry can you please reopen this PR because it was removed from master? |
|
will be handle in #1099 |
|
Do you want me to create a new pull request or you don't want to use my
commits at all?
On Tue, 7 Feb 2017, 07:07 Alexandre Chatiron, ***@***.***> wrote:
will be handle in #1099 <#1099>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1084 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA11YAeNulVloBbatjRvgGoWHXWVT1PVks5rZ_wjgaJpZM4LeYgX>
.
--
Anton
|
|
@angryziber just look at #1099, I rebase your PR and merged it in master. |
Instead of slow additional enhancement in
LocalvariablesNamesEnhancer.Java 7 is still supported, but when running on Java 8, there will be less enhancement because Java 8 standard
Method.getParameters()will be used to get parameter names.Later, if we drop support for Java 7 in some next release, we can just delete
LocalvariablesNamesEnhancerJava7class.LocalvariablesNamesEnhancerJava7is just a copy of oldLocalvariablesNamesEnhancer. PlainLocalvariablesNamesEnhancernow has the signature enhancement code removed.