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

Upgrade gradle and fix tests #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matozoid
Copy link

I thought I'd check this out and run it, but gradle immediately gave a cryptic error message. Googling suggested I upgrade Gradle, so I did. After fixing things here and there I found out that the tests were broken. The grammar and the tests do not match, so I fixed things in the grammar and in the tests. The plugin can now be built, tested, and run again.

@@ -5,11 +5,11 @@ buildscript {
}

plugins {
id "org.jetbrains.intellij" version "0.4.2"
id "org.jetbrains.intellij" version "1.1.3"
Copy link
Author

Choose a reason for hiding this comment

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

Had to go along with the gradle upgrade.

}

wrapper {
gradleVersion = '5.2.1'
gradleVersion = '7.1.1'
Copy link
Author

Choose a reason for hiding this comment

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

Picked the latest so that it will hopefully keep working for the coming years.


generateGrammarSource {
arguments += ["-package", "org.antlr.jetbrains.sample.parser", "-Xexact-output-dir"]
}
Copy link
Author

Choose a reason for hiding this comment

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

Odd stuff. Replaced with a single package header in the grammar (not perfect either) and now the files are generated in the correct package with the correct package statement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's a downside to this method, but I can't remember what it is 🤔 .

Copy link
Author

Choose a reason for hiding this comment

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

The upside is that you can open the generated files in an IDE and not get compile errors :-)
I'm happy to revert it though, just say the word.


/** The start rule must be whatever you would normally use, such as script
* or compilationUnit, etc...
*/
script
: vardef* function* statement* EOF
: (vardef|function|statement)* EOF
Copy link
Author

Choose a reason for hiding this comment

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

This makes a lot more sense and fixes a few tests.

import java.util.Collection;

public class TestXPath extends ParsingTestCase {

public static final String TEST_SAMPLE = "src/test/java/org/antlr/jetbrains/adaptor/test/test.sample";
public static final String BUBBLESORT_SAMPLE = "src/test/java/org/antlr/jetbrains/adaptor/test/bubblesort.sample";
Copy link
Author

Choose a reason for hiding this comment

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

The testfiles couldn't be found, the paths were all wrong.

String code = "var x = 1";
String output = code;
String xpath = "/script/statement";
String xpath = "/script";
Copy link
Author

Choose a reason for hiding this comment

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

vardefs are not under statement (anymore?)

@parrt parrt requested a review from bjansen July 14, 2021 21:17
@parrt
Copy link
Member

parrt commented Jul 14, 2021

Heh, cool. Could you also sign the contributors.txt file? thanks!

@@ -5,6 +5,6 @@ func f(x:[]) {

func g(x:int, y:float) { }

func h() : boolean { return false; }
func h() : boolean { return false }
Copy link
Author

Choose a reason for hiding this comment

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

The comma isn't allowed there (or is this an implicit test for invalid syntax?)

@matozoid
Copy link
Author

Okay, signed it!

@parrt
Copy link
Member

parrt commented Jul 15, 2021

I will have to wait for @bjansen to have time to review this as I've been out of the loop for too long. Sorry!

@TysonMN
Copy link

TysonMN commented Apr 16, 2023

@bjansen, do you have time to review this RP?

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.

4 participants