Skip to content

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix codestyle errors #3146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion webapp/src/Entity/Contest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,9 @@ public function validate(ExecutionContextInterface $context): void
{
foreach (['Activate', 'Deactivate', 'Start', 'End', 'Freeze', 'Unfreeze'] as $timeString) {
$tmpValue = $this->{'get' . $timeString . 'timeString'}();
if ($tmpValue === null) continue;
if ($tmpValue === null) {
continue;
}
Copy link
Member

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.

try {
$this->checkValidTimeString($tmpValue);
} catch (Exception $e) {
Expand Down
3 changes: 1 addition & 2 deletions webapp/src/Service/DOMJudgeService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1801,8 +1801,7 @@ public function checkNewVersion(): string|false {
}
}
}
}
elseif ($this->config->get('check_new_version', false) === UpdateStrategy::Strategy_major_release) {
} elseif ($this->config->get('check_new_version', false) === UpdateStrategy::Strategy_major_release) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} 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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

@vmcj vmcj Oct 8, 2025 •
edited
Loading

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.

/* Steer towards the latest version directly
* So the expected path would be:
* DJ6.0.0 -> DJ9.1.2
Expand Down
2 changes: 1 addition & 1 deletion webapp/src/Service/EventLogService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ public function addMissingStateEvents(Contest $contest): void
$this->dj->withAllRoles(function () use ($url, &$awards) {
$response = $this->dj->internalApiRequest($url);
if (!empty($response)) {
$awards = Utils::jsonDecode($response);
$awards = Utils::jsonDecode($response);
}
});
foreach ($awards as $award) {
Expand Down
4 changes: 2 additions & 2 deletions webapp/src/Utils/Utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,10 +1082,10 @@ public static function extendMaxExecutionTime(int $minimumMaxExecutionTime): voi
/**
* 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
Copy link
Member

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.

{
$status = ob_get_status();
if ( empty($status) || ($status['flags'] & PHP_OUTPUT_HANDLER_CLEANABLE) ) {
if (empty($status) || ($status['flags'] & PHP_OUTPUT_HANDLER_CLEANABLE)) {
return ob_flush();
}
return false;
Expand Down
Loading