-
Notifications
You must be signed in to change notification settings - Fork 24
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
GH-370: Update N4JS to Oxygen and Xtext 2.13 #637
Conversation
@@ -57,7 +57,7 @@ static private boolean isZeroLiteral(Expression expr) { | |||
if (expr instanceof NumericLiteral) { | |||
NumericLiteral numLit = (NumericLiteral) expr; | |||
BigDecimal litValue = numLit.getValue(); | |||
return litValue.equals(0); | |||
return litValue.equals(new BigDecimal(0)); |
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.
You may want to use BigDecimal.ZERO instead of the constructor.
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.
done
@@ -144,7 +144,7 @@ class N4JSAntlrGeneratorFragment2 extends N4AntlrGeneratorFragment2 { | |||
} | |||
} | |||
|
|||
private def boolean hasSyntheticTerminalRule() { | |||
protected override boolean hasSyntheticTerminalRule() { |
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.
Is it different from the impl in the super class?
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.
Yes it is. This method is now a protected
method in the super class XtextAntlrGeneratorFragment2
@@ -187,7 +187,7 @@ class ES_07_08_3_NumericLiteralEsprimaTest extends AbstractParserTest { | |||
val script = '0O127;'.parseSuccessfully | |||
val statement = script.scriptElements.head as ExpressionStatement | |||
val number = statement.expression as OctalIntLiteral | |||
assertEquals(0127, number.toInt) | |||
assertEquals(87, number.toInt) |
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 recommend to keep the octal literal in the expectation
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.
@szarnekow Thanks for your review! Keeping the octal literal cause the assert to fail!
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.
Interesting. I found the change in the Xbase codebase, that caused octal literals to work and stop working. There is such thing as a side-effect-free bugfix :)
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.
so, are we keeping 87
or are we waiting for xbase update and then we go with the original test?
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.
Xbase never had octal number literals, it only was a bug in the compiler
From the spec:
Number Literals
Xbase supports roughly the same number literals as Java with a few notable differences.
As in Java 7, you can separate digits using _ for better readability of large numbers.
An integer literal represents an int, a long (suffix L) or even a BigInteger (suffix BI).
There are no octal number literals.
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.
So there won't be a fix. We either have to keep the 87, or use Integer.parseInt(127, 8)
which I'd prefer
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.
done
val scriptAsString = CharStreams.toString[| | ||
new InputStreamReader(class.getResourceAsStream("InfiniteComputationTest_02.txt")) | ||
] | ||
val fileURL = class.getResource("InfiniteComputationTest_02.txt") |
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.
Looks like you want to have a utiliy that does these three lines consistently. Also there is Resources.toString
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 hint!
}, Charsets.UTF_8)); | ||
|
||
"}\n").getBytes(Charsets.UTF_8)); | ||
zipOutputStream.close(); |
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.
try - finally or try-with-resource
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.
done
storageURI="scope://Workspace"> | ||
<choice | ||
value="N4JS_Neon.2.GH-18" | ||
label="N4JS_Neon.2.GH-18"/> | ||
value="N4JS_Oxygen.2.GH-369" |
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.
or just N4JS_Oxygen
@@ -514,8 +534,8 @@ | |||
launcher="CreateConfigFileForSettingDefaultProduct"> | |||
<description>Launch that creates a config.ini</description> | |||
</setupTask> | |||
<stream name="master" | |||
label="master (Oxygen)"/> | |||
<stream name="GH-370" |
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.
change back to master
before merging
@@ -1,15 +1,16 @@ | |||
<?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
<?pde version="3.8"?> | |||
<target name="Generated target-platform N4JS:N4JS_Neon.2.GH-18" sequenceNumber="135"> | |||
<target name="Generated target-platform N4JS:N4JS_Oxygen.2.GH-369" sequenceNumber="141"> |
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.
as with Oomph setup, change to Generated target-platform N4JS:N4JS_Oxygen" sequenceNumber="141
@@ -9,7 +9,7 @@ Require-Bundle: org.eclipse.n4js, | |||
org.eclipse.n4js.hlc, | |||
org.eclipse.n4js.hlc.base, | |||
org.junit, | |||
com.google.guava;bundle-version="15.0.0", | |||
com.google.guava, |
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.
don't we need version in at least one place for Oomph to properly generate target platform file?
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.
The generated Guava version is 18
@@ -187,7 +187,7 @@ class ES_07_08_3_NumericLiteralEsprimaTest extends AbstractParserTest { | |||
val script = '0O127;'.parseSuccessfully | |||
val statement = script.scriptElements.head as ExpressionStatement | |||
val number = statement.expression as OctalIntLiteral | |||
assertEquals(0127, number.toInt) | |||
assertEquals(87, number.toInt) |
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.
so, are we keeping 87
or are we waiting for xbase update and then we go with the original test?
@@ -56,6 +56,10 @@ Contributors: | |||
id="org.eclipse.wst.xml_ui.feature" | |||
version="0.0.0"/> | |||
|
|||
<includes | |||
id="org.eclipse.epp.mpc" | |||
version="0.0.0"/> |
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.
shouldn't this be added to the N4JS__IDE.launch
?
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.
done
Task:
#369
#370
This PR updates N4JS IDE to Oxygen and Xtext 2.13