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.

Sign up for GitHub

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

Jump to bottom

[Rector] Apply BooleanInIfConditionRuleFixerRector #7951

Merged
merged 20 commits into from
Oct 16, 2023
Merged

[Rector] Apply BooleanInIfConditionRuleFixerRector #7951

merged 20 commits into from
Oct 16, 2023

Conversation

samsonasik
Copy link
Member

Description

@kenjis @paulbalandan here based on #7943 (review)

I apply BooleanInIfConditionRuleFixerRector to ensure pass bool to if cond.

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 samsonasik mentioned this pull request Sep 18, 2023
Merged
1 task
kenjis
kenjis reviewed Sep 18, 2023
View reviewed changes
system/Debug/Exceptions.php Outdated Show resolved Hide resolved
@kenjis kenjis added the refactor Pull requests that refactor code label Sep 18, 2023
@samsonasik
Copy link
Member Author

@kenjis updated 👍

@samsonasik
Copy link
Member Author

phpstan baseline regenerated 👍

kenjis
kenjis reviewed Sep 18, 2023
View reviewed changes
system/Config/Factories.php Outdated Show resolved Hide resolved
@samsonasik
Copy link
Member Author

@kenjis updated 👍

@samsonasik
Copy link
Member Author

should be ready for merge 👍

kenjis
kenjis previously approved these changes Sep 19, 2023
View reviewed changes
paulbalandan
paulbalandan reviewed Sep 19, 2023
View reviewed changes
system/BaseModel.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Show resolved Hide resolved
system/BaseModel.php Show resolved Hide resolved
system/CLI/Commands.php Outdated Show resolved Hide resolved
system/Cache/Handlers/PredisHandler.php Show resolved Hide resolved
system/Pager/Pager.php Show resolved Hide resolved
system/Test/FeatureTestCase.php Outdated Show resolved Hide resolved
system/Test/FeatureTestTrait.php Outdated Show resolved Hide resolved
system/View/View.php Outdated Show resolved Hide resolved
system/View/View.php Outdated Show resolved Hide resolved
@kenjis kenjis dismissed their stale review September 19, 2023 02:19

It would be better to remove conditions that are not necessary.

@ddevsr ddevsr added stale Pull requests with conflicts and removed stale Pull requests with conflicts labels Sep 20, 2023
@github-actions
Copy link

👋 Hi, @samsonasik!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@github-actions github-actions bot added the stale Pull requests with conflicts label Sep 22, 2023
@MGatner
Copy link
Member

MGatner commented Sep 26, 2023

@samsonasik I think this is a good direction to go but will need some more individual attention to the changes. Please review @paulbalandan and @kenjis suggestions.

@samsonasik
Copy link
Member Author

@kenjis @paulbalandan the type check is based on type defined in param, so that check fix also phpstan, except, we use assert before if to enforce type.

@samsonasik samsonasik force-pushed the boolean-in-if branch 2 times, most recently from 7fc6c79 to 15e91b2 Compare October 2, 2023 00:31
@ddevsr ddevsr removed the stale Pull requests with conflicts label Oct 16, 2023
@kenjis
Copy link
Member

kenjis commented Oct 16, 2023

@samsonasik Can you update the PR branch, and run rector again?

samsonasik and others added 4 commits October 16, 2023 11:28
@samsonasik
[Rector] Apply BooleanInIfConditionRuleFixerRector
83ebb79
@samsonasik @kenjis
use compare string for ini_get('display_errors')
8e288d2 
Co-authored-by: kenjis <kenji.uui@gmail.com>
@samsonasik @kenjis
use compare string for ini_get('display_errors')
41ecd8b 
Co-authored-by: kenjis <kenji.uui@gmail.com>
@samsonasik @kenjis
Update system/Config/Factories.php
2d3327f 
Co-authored-by: kenjis <kenji.uui@gmail.com>
@samsonasik
Copy link
Member Author

@kenjis done 👍

kenjis
kenjis reviewed Oct 16, 2023
View reviewed changes
system/HTTP/URI.php Outdated Show resolved Hide resolved
kenjis
kenjis reviewed Oct 16, 2023
View reviewed changes
system/HTTP/URI.php Outdated Show resolved Hide resolved
samsonasik and others added 2 commits October 16, 2023 12:42
@samsonasik @kenjis
add empty string check
3803fbc 
Co-authored-by: kenjis <kenji.uui@gmail.com>
@samsonasik @kenjis
add empty string check
faa0c99 
Co-authored-by: kenjis <kenji.uui@gmail.com>
@samsonasik
Copy link
Member Author

@kenjis updated 👍

@kenjis
Copy link
Member

kenjis commented Oct 16, 2023

Please run to remove ignoreErrors.

$ vendor/bin/phpstan analyze --generate-baseline phpstan-baseline.php

@kenjis
Copy link
Member

kenjis commented Oct 16, 2023

Run vendor/bin/rector process system/CLI/CLI.php first, and run phpstan to update the baseline.

@samsonasik
Copy link
Member Author

@kenjis done 👍

@kenjis
Copy link
Member

kenjis commented Oct 16, 2023

@samsonasik Thank you! 👍
PHPStan and Rector check have passed.
I'm going to merge this after all checks have passed.

@samsonasik
Copy link
Member Author

Ready to merge 👍

@kenjis kenjis merged commit 431748b into codeigniter4:develop Oct 16, 2023
60 checks passed
@samsonasik samsonasik deleted the boolean-in-if branch October 16, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbalandan paulbalandan paulbalandan left review comments

@kenjis kenjis kenjis left review comments

@MGatner MGatner Awaiting requested review from MGatner

Assignees
No one assigned
Labels
refactor Pull requests that refactor code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samsonasik @MGatner @kenjis @paulbalandan @ddevsr