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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(match): Support empty lines and comments in match expressions (closes #1956) #2201

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

claytonrcarter
Copy link
Contributor

@claytonrcarter claytonrcarter commented Aug 14, 2023

What does this do

  • preserves empty lines between match arms
  • collapses multiple empty lines between match arms into one empty line
  • prints leading comments between match arms (Comments in match statement throw error聽#1956)
  • prints trailing comments after match arms
    • this honors trailingCommaPHP option, though the tests for comments don't use that option, so there's no test coverage for it
  • adds tests for new and changed behavior
  • all changes have been formatted 馃槈

Current behavior on main

  • all empty lines between arms are discarded
  • leading and trailing comments trigger error: "Error: Comment "<comment>" was not printed. Please report this error!"

Hi, I've been using prettier/plugin-php for years, but this is my first contribution. I have no idea if I have completed all PR requirements, nor if my solution is following all of the idioms,, so please let me know and I'll glady update this.

This was a pretty easy to change once I learned how the parser and printer everything fit together. The upstream parser was already including comments in the AST for match, but the printer was not making use of them. I tried to support empty lines and comments as they are handled for function params, and I was mostly successful. The only part that I couldn't get is comments that are followed by an empty line:

  • in function args, if there is an empty line between the comment and the arg, it is preserved
  • in this PR, empty lines between a comment and the next match arm are discarded
  • I'm not sure how important this is, though, and welcome your input!

Thank you!

Example 1: no comments

Input

match($v) {

    'a' => 1,


    'b' => 2,

    default => null
};

Output

match ($v) {
    "a" => 1,

    "b" => 2,

    default => null,
};

Current behavior on main

match ($v) {
    "a" => 1,
    "b" => 2,
    default => null,
};

Example 2: with comments

Input

match($v) {

    // first comment

    'a' => 1,


    /*
     * second comment
     */

    'b' => 2,
    default => null            // final comment
};

Output

match ($v) {
    // first comment
    "a" => 1,

    /*
     * second comment
     */
    "b" => 2,
    default => null, // final comment
};

Current behavior on main

Error: Comment "// first comment" was not printed. Please report this error!

@claytonrcarter claytonrcarter changed the title fix: Support empty lines and comments in match expressions (close #1956) fix(match): Support empty lines and comments in match expressions (close #1956) Aug 14, 2023
@claytonrcarter claytonrcarter changed the title fix(match): Support empty lines and comments in match expressions (close #1956) fix(match): Support empty lines and comments in match expressions (closes #1956) Aug 14, 2023
@claytonrcarter
Copy link
Contributor Author

Sorry for the false start. I just fixed the let/const.

@czosel
Copy link
Collaborator

czosel commented Aug 15, 2023

No worries at all! I鈥檒l try to take a closer look at this soon.

Copy link
Collaborator

@czosel czosel left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for this excellent contribution 馃挴

@czosel czosel merged commit f09c383 into prettier:main Aug 16, 2023
6 checks passed
@czosel
Copy link
Collaborator

czosel commented Aug 16, 2023

Released in v0.19.7 馃帀

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

2 participants