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

fix: namespace followed by inline node #1265

Merged
merged 1 commit into from
Jan 2, 2020
Merged

fix: namespace followed by inline node #1265

merged 1 commit into from
Jan 2, 2020

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented Jan 1, 2020

Fixes #1052

Copy link
Collaborator

@loilo loilo left a comment

Choose a reason for hiding this comment

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

This should also take into account the case of leading comments. It's not exactly what the issue described, but it's a related edge case.

Consider the following snippet:

<?php
namespace foo;

// comment
?>

<!-- HTML comment -->

This currently (before applying the PR) creates the following invalid code:

<?php
namespace foo;

<?php
// comment
?>

<!-- HTML comment -->

With the PR, different invalid code is generated:

<?php
namespace foo;<?php
// comment
?>

<!-- HTML comment -->

The expected behavior would be for the opening <?php tag to just be omitted in that case.

As far as I can tell (but I don't fully grasp the code), the following line should be responsible for that:

isFirstNode ? openTag : "",

@czosel
Copy link
Collaborator Author

czosel commented Jan 2, 2020

Thanks for the review @loilo! I added a fix.

@czosel
Copy link
Collaborator Author

czosel commented Jan 2, 2020

@loilo good to go?

@loilo loilo self-requested a review January 2, 2020 22:26
@loilo
Copy link
Collaborator

loilo commented Jan 2, 2020

Seems to generate valid code now as far as I can tell. 👍

I've found another somewhat related thing, but it was not introduced by this PR, so it should not block merging.

However, for completeness: The following code...

<?php
namespace Foo;

bar();
?>

<!-- HTML comment -->

...is actually made uglier by Prettier. 😅

<?php
namespace Foo;

bar(); ?>

<!-- HTML comment -->

@czosel czosel merged commit cee7f95 into master Jan 2, 2020
@alexander-akait alexander-akait deleted the namespace-inline branch January 3, 2020 13:35
@alexander-akait
Copy link
Member

👍

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.

Invalid code when namespace is followed by HTML comment
3 participants