-
Notifications
You must be signed in to change notification settings - Fork 278
Fix codestyle errors #3146
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
base: main
Are you sure you want to change the base?
Fix codestyle errors #3146
Conversation
} | ||
} | ||
elseif ($this->config->get('check_new_version', false) === UpdateStrategy::Strategy_major_release) { | ||
} elseif ($this->config->get('check_new_version', false) === UpdateStrategy::Strategy_major_release) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} elseif ($this->config->get('check_new_version', false) === UpdateStrategy::Strategy_major_release) { | |
} elseif ( | |
$this->config->get('check_new_version', false) === UpdateStrategy::Strategy_major_release | |
) { |
I do think this is only slightly better. I would keep the original line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the line doesn't feel too long but if others think it does, we should come up with a way to make it shorter. I really don';t think the original, which saves 2 characters but has the }
weirdly on its own line, makes it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these enums are always written like UpdateStrategy::Strategy_major_release
, then why does it contain "strategy" twice? Removing that would fix this case and in general produce shorter lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense and is indeed beter to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative is to shorten UpdateStrategy to US for this inclusion. I think we shouldn't change this though.
if ($tmpValue === null) continue; | ||
if ($tmpValue === null) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that short statements like this are shorter and clearer as is.
* Call ob_flush() unless the top-level output buffer does not allow it. | ||
*/ | ||
public static function ob_flush_if_possible(): bool | ||
public static function ob_flush_if_possible(): bool // phpcs:ignore PSR1.Methods.CamelCapsMethodName.NotCamelCaps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this one in slack, but I see it's already annotated. I wrote it like this on purpose snice it wraps ob_flush
.
No description provided.