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

WIP: Refactor handling of code blocks with just comments #513

Closed

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented Aug 4, 2018

Fixes #509

@czosel
Copy link
Collaborator Author

czosel commented Aug 4, 2018

I tried fixing #509 and got the feeling that we might make our lives much easier by creating new AST nodes for code blocks that contain just comments, so I gave it a try. It seems to work quite well, but I still have issues with linebreaks/whitespace introduced by printLines.
@evilebottnawi Do you agree that the new AST nodes make it much easier? Do you have an idea how to fix the whitespace issues?

@alexander-akait
Copy link
Member

@czosel i can look on this at Monday, little vacation, also we have some regression after last commit (comments), i will create issue. Also i think we should create issue in parser, maybe new option for this or modify standard behavior.

@czosel
Copy link
Collaborator Author

czosel commented Aug 4, 2018

@evilebottnawi no rush, enjoy your vacation 😉

@czosel czosel force-pushed the refactor-code-blocks-only-comments branch from c11de07 to f7ffd41 Compare August 4, 2018 20:02
@@ -184,15 +184,11 @@ exports[`blank_lines.php 1`] = `

// Other comment
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<?php
// The first line of a comment
<?php // The first line of a comment
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

: " ";
afterOpenTag =
between[2].includes("\n") &&
firstNestedChildNode.kind !== "lonelyComment"
Copy link
Member

Choose a reason for hiding this comment

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

Bug with extra here

@alexander-akait
Copy link
Member

@czosel can you add test

<?php
if (true) {
    ?>inline<?php
    // comment
}

from #517

@alexander-akait
Copy link
Member

To be honest, I'm not sure that the idea of creating new ast nodes is good. Can you add link on issue in source code?

@czosel
Copy link
Collaborator Author

czosel commented Aug 9, 2018

@evilebottnawi done! I agree that the "post-production" AST manipulation doesn't seem very clean. The printer code is quite clean though, and it seems to me that any other approach without parser changes is not working/really hard to maintain. We should try to keep "our" AST closely aligned with what the PHP parser will do in the future, then migration will be easy.

@alexander-akait
Copy link
Member

@czosel let's add test from #526

@alexander-akait
Copy link
Member

Also need fix space and we can merge

@alexander-akait
Copy link
Member

@czosel Please read #532 (comment). Let's search solution without new ast nodes (fix it in parser or prepare text for parser).

@czosel
Copy link
Collaborator Author

czosel commented Aug 10, 2018

@evilebottnawi agreed.

@czosel czosel closed this Aug 10, 2018
@czosel czosel deleted the refactor-code-blocks-only-comments branch August 10, 2018 19:25
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.

2 participants