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

Improve ASTNode#to_s output of parenthesized expression #9637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

makenowjust
Copy link
Contributor

This fix does not have to be needed. However, it benefits to reduce user surprise in many cases (e.g. p! macro output).

macro show(e)
  {% p e %}
end

show((1; 2))
show(begin
  (1 + 2)
end)

# Before:
#   (1
#   2)
#   begin
#   (  1 + 2)
#   end

# After:
#   (1; 2)
#   begin
#     (1 + 2)
#   end

Thank you.

This fix does not have to be needed. However, it benefits to reduce user
surprise in many cases (e.g. `p!` macro output).
@asterite
Copy link
Member

What's the output of:

show((1
2))

?

@makenowjust
Copy link
Contributor Author

@asterite (1: 2). I assumed it is rarer than (1: 2).

@asterite
Copy link
Member

Sorry, I didn't understand what yo mean.

But my point is, expressions separates by newline or comma are equivalent. We don't know what the user wrote because the AST doesn't fully capture this. So I don't think this is very important to have. More importantly, we can never get it right unless we add information about whether semicolons were used or not. But if it's only for this, then in my mind it's not worth it.

@makenowjust
Copy link
Contributor Author

Current output is parsable by machine, but it looks very weird: the output has many useless spaces and indentation is almost broken.

Fortunately this problem can be fixed easily. Minimal lines diff gets surely improvement, so I think this can be acceptable.

@makenowjust
Copy link
Contributor Author

Isn't this modest improvement really acceptable?

@straight-shoota
Copy link
Member

The example looks nice, but the expression separator is a questionable improvement. It just shifts from one unexpected output to another (in the opposite case). Perhaps semicolon-separated expressions are more common for this so it might be a slight improvement to do it this way. But when expressions happen to be longer (and separated by newlines in the original code) it's really not a great ide to put them in a single line.

I'm totally in for the indentation fix, though. But the current output actually strips the parenthesis, so there's no error in the visual output:

begin
  1 + 2
end

See https://carc.in/#/r/9wv9

@makenowjust
Copy link
Contributor Author

makenowjust commented Nov 2, 2020

@straight-shoota It seems buggy. ASTNode#toString should not change its parsed result.

@straight-shoota
Copy link
Member

But that's not possible currently, because the information is lost at the parser. We could consider adding data on whether expressions are separated by newline or semi colon to the AST node. That would fix it. Not sure it's worth it, though.

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.

None yet

3 participants