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

chore: add more trailing commas in more places #9395

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

paulbalandan
Copy link
Member

Description
Done thru the trailing_comma_in_multiline fixer.

PHP allows trailing commas in other places aside from multiline arrays:

  • function/method calls (since PHP 7.3)
  • parameters in function/method declarations (since PHP 8.0)
  • closure uses (since PHP 8.0)
  • match (since PHP 8.0)
  • array destructuring (since PHP 7.1)

This PR is a no-brainer. I add the element, run cs-fix, and do nothing else.
I am doing the changes per commit as GitHub hangs on the overall diff.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@neznaika0
Copy link
Contributor

I think it degrades readability in the parameters. The trailing comma indicates that the list of properties are not finished.

@paulbalandan
Copy link
Member Author

I think it degrades readability in the parameters. The trailing comma indicates that the list of properties are not finished.

I think the reasoning behind all these extra commas is the same reasoning behind arrays: it makes diffs cleaner if you will ever insert a new item.

@michalsn
Copy link
Member

I'm not sure - adding more parameters doesn't happen often... The cases where we add commas to exceptions are good examples where it looks awkward. So, IDK - but that may just be my habit.

@michalsn michalsn requested review from samsonasik and kenjis January 10, 2025 13:34
Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

it seems it only apply on multi lines 👍 , looks good for me

This comment was marked as resolved.

@github-actions github-actions bot added the stale Pull requests with conflicts label Jan 11, 2025
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I will learn to live with it, eventually 😅

@paulbalandan paulbalandan removed the stale Pull requests with conflicts label Jan 13, 2025
@paulbalandan paulbalandan merged commit c3ac0f9 into codeigniter4:develop Jan 13, 2025
41 checks passed
@paulbalandan paulbalandan deleted the more-trailing-commas branch January 13, 2025 03:03
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.

4 participants