Skip to content

Conversation

tom-james-watson
Copy link

Non-pretty (existing functionality):

> console.log(antlr4.tree.Trees.toStringTree(tree, tree.parser.ruleNames));
(parse (sql_stmt_list (sql_stmt (select_stmt (select_core select (result_column (expr (column_name (any_name id)))) , (result_column (expr (column_name (any_name first_name)))) from (table_or_subquery (table_name (any_name users)))))) ;) <EOF>)

Pretty:

> console.log(antlr4.tree.Trees.toStringTree(tree, tree.parser.ruleNames, null, true));
(parse 
 (sql_stmt_list 
  (sql_stmt 
   (select_stmt 
    (select_core 
     select    
     (result_column 
      (expr 
       (column_name 
        (any_name 
         id
        )
       )
      )
     )    
     ,    
     (result_column 
      (expr 
       (column_name 
        (any_name 
         first_name
        )
       )
      )
     )    
     from    
     (table_or_subquery 
      (table_name 
       (any_name 
        users
       )
      )
     )
    )
   )
  ) 
  ;
 )
 <EOF>
)

@tom-james-watson
Copy link
Author

None of those instances need an else. They are simply adding newlines/indentation where necessary.

@ericvergnaud
Copy link
Contributor

None of those instances need an else. They are simply adding newlines/indentation where necessary.

Hi, they do for clarity.

@tom-james-watson
Copy link
Author

I can't say I agree that that would make anything clearer. I can do it, but it will simply mean that there is unnecessary duplication.

@tom-james-watson
Copy link
Author

Instead of

let res = "(" + s + " ";
if (prettyPrint) {
  res = `\n${" ".repeat(indentLvl)}${res}`;
}

You'd have

let res;
if (prettyPrint) {
  res = `\n${" ".repeat(indentLvl)}(${s} `;
} else {
  res = "(" + s + " ";
}

@ericvergnaud
Copy link
Contributor

Instead of

let res = "(" + s + " ";
if (prettyPrint) {
  res = `\n${" ".repeat(indentLvl)}${res}`;
}

You'd have

let res;
if (prettyPrint) {
  res = `\n${" ".repeat(indentLvl)}(${s} `;
} else {
  res = "(" + s + " ";
}

or:

const res = prettyPrint ? \n${" ".repeat(indentLvl)}(${s} : "(" + s + " ";

another option would be to have toStringTree dispatch to toUglyStringTree/toPrettyStringTree such that the maintainer does not have to think twice about side effects when evolving the formatting

@KvanTTT
Copy link
Member

KvanTTT commented Jul 21, 2021

I suggest not using parentheses form for the prettified tree:

parse 
 sql_stmt_list 
  sql_stmt 
   select_stmt 
    select_core 
     select    
     result_column 
      expr 
       column_name 
        any_name 
         id
     ,    
     result_column 
      expr 
       column_name 
        any_name 
         first_name
     from    
     table_or_subquery 
      table_name 
       any_name 
        users
  ;
 <EOF>

Also, I suggest adding a similar method to all runtimes, not only to JavaScript. See #1502

@tom-james-watson
Copy link
Author

I was just going off the similar PR that was merged for C++: #2505

@KvanTTT
Copy link
Member

KvanTTT commented Jul 21, 2021

Maybe it makes sense to add a method for prettified output (my suggestion above) and method that outputs resulting JSON that can be easily serialized/deserialized.

@parrt
Copy link
Member

parrt commented Dec 28, 2021

Have you guys come to a consensus?

@KvanTTT
Copy link
Member

KvanTTT commented Dec 28, 2021

If add the feature to JavaScript target it should be added to all other targets as well for consistency.

@parrt
Copy link
Member

parrt commented Dec 28, 2021

If add the feature to JavaScript target it should be added to all other targets as well for consistency.

yeah that sounds like a big change that I don't want to focus on. Maybe we just make that function available as part of the documentation for JavaScript?

@KvanTTT
Copy link
Member

KvanTTT commented Dec 28, 2021

Maybe we just make that function available as part of the documentation for JavaScript?

I don't know but I'm not sure it's a very demanded feature for the current time. Especially considering that tree in any format can be obtained using a utility method in runtime.

Also, not sure about lisp-like format. I would think about something serializable at first (JSON).

@parrt
Copy link
Member

parrt commented Dec 28, 2021

Also, not sure about lisp-like format. I would think about something serializable at first (JSON).

The lisp thing is good for showing the structure but I still prefer the GUI visualization using the intellij plug-in (all hail jetbrains!)

@KvanTTT
Copy link
Member

KvanTTT commented Dec 28, 2021

The lisp thing is good for showing the structure

Maybe it's better for showing than JSON. But my suggested format with indents and without parentheses is also good and is shorter (BTW, I took it from Kotlin specification repository). That's why I don't want to rush with accepting a similar request because pretty print formats may differ and may be subjective. On the other hand, JSON format can be converted to almost everything else.

@tom-james-watson
Copy link
Author

I'm no longer using antlr at all and have no bandwidth to address this, so will close out. Of course feel free to repurpose my changes if you want them.

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