Introduce data transfer objects. #2306
Conversation
nickygerritsen
force-pushed
the
data-transfer-objects
branch
from
58a46e3 to
8439b6c
Compare
| */ | ||
| protected function getAwardsData(Request $request, string $requestedType = null): ?array | ||
| protected function getContestAndScoreboard(Request $request): array |
There was a problem hiding this comment.
I didn't check details but this change looks fishy to me...
There was a problem hiding this comment.
Git diff is horrible. I extracted a method since I changed the existing method that either returned one award or an array of awards in two methods
| 'filename' => $k, | ||
| 'content' => base64_encode($inout[$k]), | ||
| ]; | ||
| $result[] = new JudgehostFile( |
There was a problem hiding this comment.
| $result[] = new JudgehostFile( | |
| $result[] = new JudgehostFile( |
| 'content' => base64_encode($file->getFileContent()), | ||
| 'is_executable' => $file->isExecutable(), | ||
| ]; | ||
| $result[] = new JudgehostFile( |
There was a problem hiding this comment.
| $result[] = new JudgehostFile( | |
| $result[] = new JudgehostFile( |
First of all, seems codecov.io doesn't handle PR's from forks currently since we moved it to Github, but I ran it locally. I can only tell something about the API easily. For unit tests, we don't test:
For the purpose of this PR I think we test almost everything, but this list might be a good start to improve our API coverage. |
nickygerritsen
force-pushed
the
data-transfer-objects
branch
2 times, most recently
from
a55f1bc to
d162ae1
Compare
nickygerritsen
force-pushed
the
data-transfer-objects
branch
from
d162ae1 to
05c79fa
Compare
This seems like a big PR (and it probably is), but basically what it does is it changes many (but not all, it's only a start) of our 'magic' arrays into objects, mostly using PHP 8's new constructor property promotion. This has some advantages:
Most of these changes are for the API, but not all. The API part should be extensively tested by CI, and the few UI changes should be OK.
@vmcj and me were discussing that, if this gets approval soon ™️ , he might want to run it at SWERC to see if it works. It's easy to rollback IMHO and then we will find out soon enough if everything works. But I can also understand people might find this risky. But then, given the upcoming contest season for us, I'm not sure when to merge.
Note: I created separate commits so you can see what I did but in the end I will squash everything.