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

Refactor/attach comments on node #278

Merged

Conversation

clementdessoude
Copy link
Contributor

@clementdessoude clementdessoude commented Oct 5, 2019

Fix #262 and fix #276.

Will help to fix issue #270

@clementdessoude
Copy link
Contributor Author

clementdessoude commented Oct 5, 2019

There is still an issue with handling some trailing comments. This behavior is also found in master

Input:

class T {
  void t() {
    if (e) {
      // empty
    } // comment
   else {
      
    }
  }
}

Output:

class T {

  void t() {
    if (e) {
      // empty
    } else {} // comment
  }
}

@clementdessoude clementdessoude changed the title Refactor/attach comments node Refactor/attach comments on node Oct 5, 2019
@clementdessoude clementdessoude force-pushed the refactor/attach-comments-node branch from eaa7293 to 108f5b3 Compare October 23, 2019 09:16
@clementdessoude clementdessoude force-pushed the refactor/attach-comments-node branch from 00b96d2 to 9a623e6 Compare November 17, 2019 00:08
@clementdessoude clementdessoude force-pushed the refactor/attach-comments-node branch from 376dcd0 to 81753d4 Compare November 17, 2019 13:00
@clementdessoude clementdessoude marked this pull request as ready for review November 17, 2019 13:05
@clementdessoude
Copy link
Contributor Author

clementdessoude commented Nov 17, 2019

It is ready for review. As I modified the behavior in the parser, I requested a review from you @bd82. Please let mme know if you have time to review or not 😄

I think there will be some edge cases that are not correctly handled, like those I corrected in labeledStatements and ifStatements. I think it could be good to fix these cases in separate PRs, as it will be difficult to review otherwise.

@bd82
Copy link
Contributor

bd82 commented Nov 17, 2019

Hi @clement26695 I do not actually remember what was the previous behavior of comments handling and I do not have the time to go in depth in this large PR.

Can you describe what was done? what was the change and why?

@clementdessoude
Copy link
Contributor Author

Basically, I attached the comments on nodes when possible, instead of tokens. Indeed, it is much easier to process it then in the printer, due to the way the group command work in prettier.

So, if you have time to look at how we attach the comments on nodes, it would be great, but otherwise, @Shaolans could have a look, and we could try to optimise it (if necessary) in other PRs

@bd82
Copy link
Contributor

bd82 commented Nov 17, 2019

Attaching the comments on nodes instead of Tokens seems to make sense.
I looked at the code at a bit but only partially understood it, perhaps some more comments (pun intended :smile) could be useful to explain the logic e.g:

  • What is the condition for a comment to be a trailing/leading comment to be associated with a node?
  • What happens with a sequence of trailing/leading comments?
  • What are the helper data structures you are building.

@clementdessoude
Copy link
Contributor Author

You're right, it is quite unclear. Will try to clarify/document the process

@clementdessoude clementdessoude force-pushed the refactor/attach-comments-node branch from 248f888 to 4e8fe2b Compare November 18, 2019 12:18
@clementdessoude
Copy link
Contributor Author

Done ! I hope it is more understandable

@bd82
Copy link
Contributor

bd82 commented Nov 18, 2019

Done ! I hope it is more understandable

Yes makes much more sense now.

I think you could have also added info to the CSTNodes during parsing:

  • First Token in CSTNode
  • Last Token in CSTNode

Instead of creating some of these helper data structures and than perform the logic
directly on the CSTNodes and Comments array. However I do not have an opinion which approach is better.

return concat([this.visit(compilationUnit[0]), ctx.EOF[0]]);

// Do not add additional line if only comments in file
const additionalLine = isNaN(compilationUnit[0].location.startOffset)
Copy link
Member

Choose a reason for hiding this comment

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

Why a new line is required ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, it is just good practice to end a file by an empty line. But I don't know exactly why 😅

Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should not enforce it since it JHipster app do not have that additional line or at least discuss about it.
Anyway great work !

@clementdessoude clementdessoude merged commit 6354796 into jhipster:master Nov 25, 2019
@clementdessoude clementdessoude mentioned this pull request Nov 25, 2019
@clementdessoude clementdessoude deleted the refactor/attach-comments-node branch November 26, 2019 08:32
@jhaber jhaber mentioned this pull request Dec 3, 2019
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.

Switch statement indentation printWidth not enforced when leading comments are present
3 participants