adds support for Custom Token Scanner Symbols #410

gabru-md · 2020-03-10T12:57:26Z

This pull request aims at implementing the Custom Token Symbols class for Jinjava. The goal is to allow the user to specify the tokens that he/she wants to use. It can be extending using the TokenScannerSymbols abstract class and then filling the abstract methods.

A few lines inside the tree package have been changed from switch-case to if-else since switch-case allows only constant expressions to be used with them. The changes to the tests which were failing initially due to the change have also been made and it should work fine now. Furthermore a JUnit test file has also been added in support of the changes made.

related issue: #402

@KidsDontPlay perhaps this solves your problem.

cheers!

gabru-md · 2020-03-10T14:01:32Z

even after running java-prettier on my end travis seems to be failing for the same reason of unformatted code. 🤦‍♂️

boulter

Thanks for this, @gabru-md!

There's definitely some formatting changes here that I can't explain. When I build locally, it doesn't change anything. Could you revert all the changes that are just formatting-related? That will make this easier to review.

Thanks again!

boulter · 2020-03-13T20:02:35Z

src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java

@@ -51,7 +51,10 @@ public OutputNode render(JinjavaInterpreter interpreter) {
    if (interpreter.getConfig().isNestedInterpretationEnabled()) {
      if (
        !StringUtils.equals(result, master.getImage()) &&
-        (StringUtils.contains(result, "{{") || StringUtils.contains(result, "{%"))
+        (
+          StringUtils.contains(result, "{{") ||


Looks like this should use the scanner symbols as well.

I've made the changes. Although I'm not quite sure to if the {{ is to be replaced by
TOKEN_PREFIX_CHAR + TOKEN_EXPR_START_CHAR
or is it just
TOKEN_EXPR_START_CHAR + TOKEN_EXPR_START_CHAR.

Please review it once again.
Cheers!

boulter · 2020-03-13T20:03:33Z

src/main/java/com/hubspot/jinjava/tree/TreeParser.java

+    ) return tag((TagToken) token); else if (
+      token.getType() == symbols.TOKEN_NOTE()
+    ) {
+      if (!token.getImage().endsWith("#}")) {


Use TOKEN_NOTE_CHAR here?

boulter · 2020-03-13T20:04:25Z

src/main/java/com/hubspot/jinjava/tree/parse/DefaultTokenScannerSymbols.java

+  char TOKEN_TRIM_CHAR = '-';
+
+  @Override
+  public char TOKEN_PREFIX_CHAR() {


should be getTokenPrefix(). Same for the others below.

changes made!

boulter · 2020-03-13T20:05:42Z

src/main/java/com/hubspot/jinjava/tree/parse/Token.java

+      startPosition
+    ); else if (
+      tokenKind == symbols.TOKEN_EXPR_START()
+    ) return new ExpressionToken(image, lineNumber, startPosition); else if (


else if should be on a new line

boulter · 2020-03-13T20:06:23Z

src/main/java/com/hubspot/jinjava/tree/parse/Token.java

+    int lineNumber,
+    int startPosition
+  ) {
+    if (tokenKind == symbols.TOKEN_FIXED()) return new TextToken(


please always wrap bodies of if statements in { }

boulter · 2020-03-13T20:08:22Z

src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java

-  int TOKEN_TRIM = TOKEN_TRIM_CHAR;
+public abstract class TokenScannerSymbols {
+
+  public abstract char TOKEN_PREFIX_CHAR();


same comment as before

gabru-md · 2020-03-14T07:07:29Z

@boulter

while debugging the code after making the changes, I noticed something weird here.

this.expr = WhitespaceUtils.unwrap(image, "{{", "}}");

// inside the unwrap function
 return s.substring(start + prefix.length(), end - suffix.length() + 1);

the unwrap function does not care about the prefix and the suffix being passed into the function. The only purpose it serves is when the function calculates the length of the prefix and the suffix to trim the image and return the expression. Should there not be a validation inside the unwrap function that asserts a check that the correct prefix and suffix has been used in the image?

I was just wondering if it should be added as well?

boulter · 2020-03-17T15:23:12Z

@gabru-md, could you run mvn clean verify on your build so prettier runs and check it in? It's hard to see what the differences are since there's so many whitespace and formatting changes. Thanks!

gabru-md · 2020-03-17T15:50:03Z

@boulter, I executed mvn clean verify and applied the diff created by the prettier. Please see if it is reviewable now.

boulter

Looks pretty good. I have a few suggestions for changes.

boulter · 2020-03-17T16:55:03Z

src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java

@@ -48,10 +49,23 @@ public OutputNode render(JinjavaInterpreter interpreter) {

    String result = Objects.toString(var, "");

+    TokenScannerSymbols symbols = interpreter.getConfig().getTokenScannerSymbols();
+    String expressionBegins = new StringBuilder()


Could you create these once and make it accessible from the TokenScannerSymbols class?

I didn't quite get what you're trying to say. Can you please explain a little bit so that I can make the changes?

Sure. Add getExpressionStart() and getExpressionEnd() methods to TokenScannerSymbols and ensure they're only built once, rather than every time a node is rendered.

boulter · 2020-03-17T16:56:54Z

src/main/java/com/hubspot/jinjava/tree/TreeParser.java

+    } else if (token.getType() == symbols.getTokenTag()) {
+      return tag((TagToken) token);
+    } else if (token.getType() == symbols.getTokenNote()) {
+      String commentClosed = new StringBuilder()


this can also be defined once.

boulter · 2020-03-17T16:57:54Z

src/main/java/com/hubspot/jinjava/tree/parse/DefaultTokenScannerSymbols.java

+  char TOKEN_TRIM_CHAR = '-';
+
+  @Override
+  public char getTokenPrefixChar() {


all of these start with getToken so the Token part is redundant. Can you remove that part?

okay. I will remove that !

gabru-md · 2020-03-18T18:44:34Z

@boulter,
I am facing an error. As you might have noticed that I've introduced a new test file called CustomTokenScannerSymbolsTest.java. The problem that I'm facing when I try to run tests on Eclipse. When I run the CustomTokenScannerSymbolsTest.java file then the new tests seem to pass whereas when I execute all the tests in the test package then the new tests which I've created starts failing and the reason I could identify is due to the fact that the interpreter still uses
DefaultTokenScannerSymbols instead of the newly created CustomTokens.

I cannot quite seem to figure out why this behavior is happening.

boulter

Getting there!

I ran this locally and I had a lot of null pointer exceptions, so I think there's some fixes needed there.

I'm not sure about eclipse as I use IntelliJ. In any case, I would use the command line build as the source of truth.

Thanks!

boulter · 2020-03-19T14:01:45Z

src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java

+    String expressionBegins = symbols.getExpressionStart();
+    String expressionWithTag = symbols.getExpressionStartWithTag();


unnecessary variables as they are only used once.

boulter · 2020-03-19T14:02:50Z

src/main/java/com/hubspot/jinjava/tree/TagNode.java

-    stringBuilder
-      .append(TokenScannerSymbols.TOKEN_EXPR_START_CHAR)
-      .append(TokenScannerSymbols.TOKEN_TAG_CHAR);
+    stringBuilder.append(symbols.getExprStartChar()).append(symbols.getTagChar());


can you add a method to construct this string once as well?

sure. I might have missed this out in the last commit.
I will add this to the changes as well.

boulter · 2020-03-19T14:03:55Z

src/main/java/com/hubspot/jinjava/tree/TagNode.java

-      .append(TokenScannerSymbols.TOKEN_TAG_CHAR)
-      .append(TokenScannerSymbols.TOKEN_EXPR_END_CHAR);
+
+    stringBuilder.append(symbols.getTagChar()).append(symbols.getExprEndChar());


boulter · 2020-03-19T14:05:36Z

src/main/java/com/hubspot/jinjava/tree/parse/TagToken.java

 import com.hubspot.jinjava.interpret.TemplateSyntaxException;
 import com.hubspot.jinjava.util.WhitespaceUtils;

 public class TagToken extends Token {
  private static final long serialVersionUID = -4927751270481832992L;
+  private final int TOKEN_TAG;


Suggested change

private final int TOKEN_TAG;

private final int tokenTag;

only static variables should be in upper case.

boulter · 2020-03-19T14:05:50Z

src/main/java/com/hubspot/jinjava/tree/parse/TagToken.java


  private String tagName;
  private String rawTagName;
  private String helpers;

  public TagToken(String image, int lineNumber, int startPosition) {
    super(image, lineNumber, startPosition);
+    TOKEN_TAG =


Suggested change

TOKEN_TAG =

tokenTag =

boulter · 2020-03-19T14:06:00Z

src/main/java/com/hubspot/jinjava/tree/parse/TextToken.java

 import org.apache.commons.lang3.StringUtils;

 public class TextToken extends Token {
  private static final long serialVersionUID = -6168990984496468543L;
+  private final int TOKEN_FIXED;


Suggested change

private final int TOKEN_FIXED;

private final int tokenFixed;

boulter · 2020-03-19T14:06:16Z

src/main/java/com/hubspot/jinjava/tree/parse/TextToken.java


  public TextToken(String image, int lineNumber, int startPosition) {
    super(image, lineNumber, startPosition);
+    TOKEN_FIXED =


Suggested change

TOKEN_FIXED =

tokenFixed =

boulter · 2020-03-19T14:09:11Z

src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java

+    return getTrimChar();
+  }
+
+  public String getExpressionStart() {


it would good to fill this out with all the other starting and closing sequences that we'll eventually need.

@gabru-md are all these in there now?

Yes, all the required combinations of sequences are in here right now.

boulter · 2020-03-19T14:09:42Z

src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java

+
+  public abstract char getTrimChar();
+
+  public int getPrefix() {


why are all these methods that just call abstract methods needed?

there are places when an integer is being required and there are separate places where a character is required during parsing the template. Also, it will be better if the user is allowed to fill in the functions which return char and not the ASCII value instead. Somebody who wants custom tokens will only have to fill in the tokens in form of characters and do not need to write down their ASCII values.
Also, I could've used those abstract methods instead since they can be cast to integers as well but the problem lies with the fact that when I was writing the if..else conditions then it was becoming a lot unclear since the casting had to be done during the comparison. Therefore I thought It'd be better to not mess up the code readability a lot and simply have users fill out the functions which return char and then return their int value using the getPrefix functions instead. I believe it was a better way to make changes.
I hope my explanation is fine. If you feel that it is not okay then I'd try and make the changes.

boulter · 2020-03-19T14:10:47Z

src/test/java/com/hubspot/jinjava/tree/parse/CustomTokenScannerSymbolsTest.java

+  }
+
+  @Test
+  public void testsThatCustomTokensDoesNotFail() {


can you make these rspec-style? itRendersWithCustomTokens, etc.

gabru-md · 2020-03-19T15:07:47Z

I believe that the command line builds should not fail if the same isn't failing for me in Eclipse. But since they are then I will work on it and try to get to the depth of the problem and commit a fix as soon as I am done with it. Since this does not seem to be a high-priority feature then I wish I can take the time I want to get it all fixed. I will perhaps need some more reviews from your end though and will tag you once changes seem fine to me.

gabru-md · 2020-03-22T16:58:49Z

@boulter,
please review. I've made the changes required and the tests are now passing.
cheers!

boulter

just 2 more loose ends

boulter · 2020-03-26T20:48:57Z

src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java

+    String expressionBegins = symbols.getExpressionStart();
+    String expressionWithTag = symbols.getExpressionStartWithTag();


boulter · 2020-03-26T20:49:42Z

src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java

+    return getTrimChar();
+  }
+
+  public String getExpressionStart() {


@gabru-md are all these in there now?

KidsDontPlay · 2020-03-27T17:51:30Z

@gabru-md
thanks for your effort

adds support for Custom Token Scanner Symbols

14086a6

gabru-md force-pushed the gabru-md/token-scanner-upd branch from 40674ed to 14086a6 Compare March 10, 2020 13:35

boulter requested changes Mar 13, 2020

View reviewed changes

renames functions + review changes

10bdb16

gabru-md force-pushed the gabru-md/token-scanner-upd branch from 96ec8ef to 10bdb16 Compare March 14, 2020 06:55

ran mvn clean verify + diff applied

f51d090

boulter requested changes Mar 17, 2020

View reviewed changes

gabru-md added 2 commits March 18, 2020 23:44

adds changes to TokenScannerSymbols

ce4d826

reformats CustomTokenScannerSymbolsTest.java

018d77d

boulter requested changes Mar 19, 2020

View reviewed changes

gabru-md added 2 commits March 22, 2020 22:12

adds getOrDefaultToken to Token.java

6b12da6

removes popCurrent() from Tests

313d956

gabru-md requested a review from boulter March 22, 2020 16:58

mattcoley mentioned this pull request Mar 24, 2020

New release (pending PR merge) #415

Merged

boulter reviewed Mar 26, 2020

View reviewed changes

removes unnecessary variables

9610f1a

boulter approved these changes Mar 27, 2020

View reviewed changes

boulter merged commit 9c16124 into HubSpot:master Mar 27, 2020

This was referenced Apr 1, 2020

check for null config too #418

Merged

store reference to TokenScannerSymbols in Nodes and Tokens #419

Merged

		String expressionBegins = symbols.getExpressionStart();
		String expressionWithTag = symbols.getExpressionStartWithTag();

adds support for Custom Token Scanner Symbols #410

adds support for Custom Token Scanner Symbols #410

Conversation

gabru-md commented Mar 10, 2020

gabru-md commented Mar 10, 2020

boulter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabru-md commented Mar 14, 2020

boulter commented Mar 17, 2020

gabru-md commented Mar 17, 2020

boulter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabru-md commented Mar 18, 2020

boulter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabru-md commented Mar 19, 2020

gabru-md commented Mar 22, 2020

boulter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KidsDontPlay commented Mar 27, 2020