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: Apply code quality level 31 for rector #9303

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Dec 5, 2024

Description

Applied ThrowWithPreviousExceptionRector so when throwing into a catch block, checks that the previous exception is passed to the new throw clause

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

@samsonasik
Copy link
Member Author

Ready to merge 👍

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

What is the issue with compact() that this change brings?

@samsonasik
Copy link
Member Author

That's easier to verify that the variable exists and used after defined.

@neznaika0
Copy link
Contributor

I purposely used compact() to compress the string...

@paulbalandan
Copy link
Member

paulbalandan commented Dec 6, 2024

It seems both approach emits a warning when the variable is undefined. Difference only is that compact does not give the array key of the undefined variable while the array approach includes the key but with null value instead: https://3v4l.org/pHACp#v8.4.1

I'm not sure which way we want to go, but personally I use compact for its conciseness.

@samsonasik
Copy link
Member Author

Ok, let see what other opinion for it

@michalsn
Copy link
Member

michalsn commented Dec 6, 2024

Personally, I have nothing against using compact(). It is easy to understand when reviewing the code.

@samsonasik
Copy link
Member Author

Ok, I will skip it. We can continue increase level with skip this rule.

@samsonasik samsonasik marked this pull request as draft December 6, 2024 18:12
@samsonasik samsonasik changed the title refactor: Apply code quality level 27 for rector refactor: Apply code quality level 31 for rector Dec 7, 2024
@samsonasik samsonasik marked this pull request as ready for review December 7, 2024 10:47
@samsonasik
Copy link
Member Author

I skipped CompactToVariablesRector 37e6329 , then apply level 31 that apply change with ThrowWithPreviousExceptionRector 608b869

@samsonasik samsonasik requested review from paulbalandan, neznaika0 and michalsn and removed request for neznaika0 December 7, 2024 10:48
@samsonasik
Copy link
Member Author

Ready to merge 👍

Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

Go next level)

@samsonasik
Copy link
Member Author

Thanks all for the review, let's merge 👍

@samsonasik samsonasik merged commit 124a7e6 into codeigniter4:develop Dec 7, 2024
41 checks passed
@samsonasik samsonasik deleted the refactor-apply-code-quality-level-27 branch December 7, 2024 11:18
@kenjis kenjis added the refactor Pull requests that refactor code label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants