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

Add ExceptionList property for LongParameterList #559

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andrewwheal
Copy link

I took inspiration/code from the ShortVariable rule which already had an exception list property and added it to the LongParameterList rule.

kylekatarnls
kylekatarnls previously approved these changes Jun 25, 2019
@ravage84 ravage84 added this to the 2.7.0 milestone Jun 25, 2019
Copy link
Member

@ravage84 ravage84 left a comment

Choose a reason for hiding this comment

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

@andrewwheal thank you for your enhancement.
Please add some documentation. This way, other's will find this nice enhancement, too.

@@ -42,6 +42,12 @@ public function apply(AbstractNode $node)
return;
}

$exceptions = $this->getExceptionsList();

Copy link
Member

Choose a reason for hiding this comment

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

Remove white line to move these two related code parts together. As they belong toghether and do a special thing, they can be extracted into a separate protected method.

For example:

        if ($this->parameterIsInExceptionList()) {
            return;
        }

MarkVaughn
MarkVaughn previously approved these changes Jun 25, 2019
@ravage84
Copy link
Member

ravage84 commented May 6, 2020

@andrewwheal are you still intersted in pursuing this?

Remove the empty line as requested by @ravage84
@tvbeek tvbeek dismissed stale reviews from MarkVaughn and kylekatarnls via 2db44cd September 1, 2020 19:30
@tvbeek tvbeek modified the milestones: 2.9.0, 2.10.0 Sep 2, 2020
@kylekatarnls kylekatarnls modified the milestones: 2.10.0, 2.11.0 Apr 11, 2021
@kylekatarnls kylekatarnls modified the milestones: 2.11.0, 2.12.0 Nov 27, 2021
@kylekatarnls kylekatarnls modified the milestones: 2.12.0, 2.13.0 Jun 19, 2022
@kylekatarnls kylekatarnls modified the milestones: 2.13.0, 2.14.0 Sep 10, 2022
@kylekatarnls kylekatarnls modified the milestones: 2.14.0, 2.15.0 Sep 27, 2023
@kylekatarnls kylekatarnls removed this from the 2.15.0 milestone Dec 10, 2023
@kylekatarnls kylekatarnls added this to the 2.16.0 milestone Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants