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

Split BeforeValidationInterface to separate interfaces #331

Merged
merged 8 commits into from
Oct 9, 2022

Conversation

vjik
Copy link
Member

@vjik vjik commented Oct 7, 2022

Q A
Is bugfix? ✔️
New feature?
Breaks BC? ✔️
Fixed issues #326 #329

@vjik vjik requested a review from a team October 7, 2022 11:40
@vjik vjik added the status:code review The pull request needs review. label Oct 7, 2022
skipOnEmpty: static function (mixed $value, bool $isAttributeMissing): bool {
return $isAttributeMissing || $value === '';
}
skipOnEmpty: static fn (mixed $value, bool $isAttributeMissing): bool => $isAttributeMissing || $value === ''
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is too long now. We actually decided to disable this rule completely because it also transforms multiple line static functions like that. Ideally need to configure it to transform only when the result fits max line length.

Can be disabled like that - yiisoft/yii-runner-console@ff8151d.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix line lenght in this case.

Copy link
Contributor

@arogachev arogachev left a comment

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@623628e). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #331   +/-   ##
=========================================
  Coverage          ?   95.65%           
  Complexity        ?      597           
=========================================
  Files             ?       74           
  Lines             ?     1427           
  Branches          ?        0           
=========================================
  Hits              ?     1365           
  Misses            ?       62           
  Partials          ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +172 to +175
skipOnEmpty: static fn (
mixed $value,
bool $isAttributeMissing
): bool => $isAttributeMissing || $value === ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
skipOnEmpty: static fn (
mixed $value,
bool $isAttributeMissing
): bool => $isAttributeMissing || $value === ''
skipOnEmpty: static function (mixed $value, bool $isAttributeMissing): bool {
return $isAttributeMissing || $value === '';
}

Copy link
Contributor

@arogachev arogachev Oct 8, 2022

Choose a reason for hiding this comment

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

(Optional, code style) The style before the changes is better for one-liners in my opinion.

@vjik vjik merged commit fe43ea1 into master Oct 9, 2022
@vjik vjik deleted the split-before-validation branch October 9, 2022 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
4 participants