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

Remove Extra String Concat Token #16382

Merged
merged 3 commits into from
Feb 2, 2016
Merged

Remove Extra String Concat Token #16382

merged 3 commits into from
Feb 2, 2016

Conversation

jdconrad
Copy link
Contributor

@jdconrad jdconrad commented Feb 2, 2016

Renamed no-semicolon tests to work when running gradle. Also removed the ..= token entirely from the grammar as string concatenation now uses the overloaded += operator.

@rjernst
Copy link
Member

rjernst commented Feb 2, 2016

LGTM

@@ -1,19 +1,16 @@
// ANTLR GENERATED CODE: DO NOT EDIT
package org.elasticsearch.painless;

import org.antlr.v4.runtime.CharStream;
import java.util.Set;
Copy link
Member

Choose a reason for hiding this comment

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

You might not have meant to indent that.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, antlr did that. How did it do that? Oh well, doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's auto-generated based on the Lexer file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a personal preference of mine to have the Lexer file look like this --

@header {
    import java.util.Set;
}

Instead it can be changed to look like this --

@header {
import java.util.Set;
}

However, I felt that it was more important to make the created content readable over the auto-generated content.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! That makes sense. Its cool.

Copy link
Member

Choose a reason for hiding this comment

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

We are going to have to figure out a way to make checkstyle ignore these.... Eventually!

Copy link
Member

Choose a reason for hiding this comment

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

You can <suppress checks="." files="org/elasticsearch/painless/PainlessLexer.java"/> or whatever pattern we need on the end to get any other auto-generated files (something like that, going off-the-cuff here).

@jdconrad jdconrad changed the title Fix Test Case Remove Extra String Concat Token Feb 2, 2016
jdconrad added a commit that referenced this pull request Feb 2, 2016
Remove Extra String Concat Token
@jdconrad jdconrad merged commit ce12250 into elastic:master Feb 2, 2016
@nik9000
Copy link
Member

nik9000 commented Feb 2, 2016

LGTM too

@clintongormley clintongormley removed the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Apr 5, 2016
@clintongormley clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants