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

Comment inside empty Elmish children is lost #1179

Closed
3 tasks
nojaf opened this issue Oct 2, 2020 · 4 comments · Fixed by #1675
Closed
3 tasks

Comment inside empty Elmish children is lost #1179

nojaf opened this issue Oct 2, 2020 · 4 comments · Fixed by #1675
Labels
area-trivia good first issue Long hanging fruit: easy issue to get your feet wet!

Comments

@nojaf
Copy link
Contributor

nojaf commented Oct 2, 2020

Issue created from fantomas-online

Code

a [] [
    // def
]

Result

a [] []

Problem description

Comment disappears.
Reported by @reinux in #932

Extra information

  • The formatted result breaks by code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.

Options

Fantomas Master at 09/30/2020 06:55:15 - b0dedcd

Default Fantomas configuration.


Hey @reinux, as I tried to explain in #932, please always create a new issue when it comes to bugs related to comments.
I will judge myself if they are related or not. There is no one fix that will solve all these sorts of issues so we need to tackle case by case.

That being said, this issue is actually not that hard to solve.
You can see in the Trivia tab that the comment was detected and assigned to a token node.

image

The problem here is that we don't call print content before that token in CodePrinter. Notice we are doing it after.

More information on how Fantomas technically works can be found on YouTube and in the contributing guide.
I hope I can encourage you or someone else to try and take a stab at this.

@nojaf nojaf added good first issue Long hanging fruit: easy issue to get your feet wet! area-trivia labels Oct 2, 2020
@reinux
Copy link

reinux commented Oct 2, 2020

I'm going through the videos right now, and I'm wondering: aside from cases where trivia is affected (as in this bug), is there a reason the entire output AST can't be checked against the input AST each time formatSourceString is run?

It would seem like a decent sanity check to do just to make sure there isn't a bug that's breaking code, and probably not that expensive.

@nojaf
Copy link
Contributor Author

nojaf commented Oct 3, 2020

Fantomas doesn't strictly guarantee the exact same tree. As far as I know, it never claimed to do so.
For most unit tests that will probably the case, yet there are situations where extra parenthesis are added and that leads to different nodes in the tree. For example the fix for 1183.

It is something that could be explored thought.

@reinux
Copy link

reinux commented Oct 3, 2020

Ah, interesting. I guess superfluous AST elements could be ignored for the sake of comparison, but if any cases are missed, inputs that used to work would suddenly break.

@nojaf
Copy link
Contributor Author

nojaf commented Oct 3, 2020

Yes, Fantomas doesn't require working code to be able to format it. If the syntax is valid and the compiler can construct the AST we can format it. So that is why the unit tests do validate the result after formatting. Yet that doesn't necessarily mean that the code is still working, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-trivia good first issue Long hanging fruit: easy issue to get your feet wet!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants