mirror of
https://github.com/MaSzyna-EU07/maszyna.git
synced 2026-11-11 16:57:12 +01:00
26 lines
3.2 KiB
Markdown
26 lines
3.2 KiB
Markdown
# Review PR #150 — refactor: split OnCommands from Train
|
||
|
||
Autor: @jerrrrycho · base: `master` · 39 plików, +12675/−12526 · ostatnia aktualizacja 10.10
|
||
https://github.com/MaSzyna-EU07/maszyna/pull/150
|
||
|
||
**Werdykt: Approve kierunku; Request changes — rebase i ustalenie kolejności z PR-ami dotykającymi `Train.cpp`**
|
||
|
||
## Podsumowanie
|
||
`vehicle/Train.cpp` rozbity skryptem na `vehicle/train/`: `Train.cpp`, `TrainCabSetup.cpp`, `TrainCommands{Auxiliary,Brakes,Controls,Doors,Electrical,Lights,Misc,Radio}.cpp`, `TrainSounds.cpp`, `TrainState.cpp`. `Train.h` przeniesiony do `vehicle/train/Train.h`, include'y w ~20 plikach poprawione. Przy okazji poprawki Sonara.
|
||
|
||
## Co sprawdziłem (mechanicznie)
|
||
Porównałem wszystkie linie usunięte z `Train.cpp` z liniami dodanymi w `vehicle/train/*` (bez białych znaków):
|
||
- ~12 500 linii przeniesionych 1:1; różni się ~160 linii usuniętych / ~300 dodanych, z czego większość to nagłówki licencji i `#include` w nowych plikach. ✔
|
||
- Pozostałe różnice to połączenie zagnieżdżonych `if` (S1066), np. `else if (RELEASE) { if (type == push) … }` → `else if (RELEASE && type == push)`. Sprawdziłem, że w tych miejscach to ostatnia gałąź łańcucha, a przy `EpFuseSwitch(true) && dsbPneumaticSwitch` wywołanie z efektem ubocznym zostaje po lewej. Semantyka zachowana. ✔
|
||
|
||
## Uwagi blokujące
|
||
1. **Rebase.** Od 10.10 do `master` doszły pliki, które włączają stary nagłówek — np. `world/RoadPoint.cpp` ma `#include "vehicle/Train.h"`, a PR go nie zmienia → po merge build się wysypie. Po rebase: `git grep '"vehicle/Train.h"'` musi zwracać pusto.
|
||
2. **Kolejność z innymi PR-ami.** Ten PR koliduje z każdym otwartym PR-em, który zmienia `Train.cpp`/`Train.h`: #234, #259, #261, #165, #172 (i pośrednio `feat/gl46-migration`). Proponuję ustalić z maintainerami okno: albo najpierw zmergować te małe PR-y, a potem rebase tego (skrypt da się puścić ponownie), albo odwrotnie i poprosić autorów o rebase. Skoro podział robi skrypt, warto go dołączyć do PR-a (np. `tools/`), żeby dało się go powtórzyć na świeżym `master`.
|
||
|
||
## Pozostałe uwagi
|
||
3. **Rozdzielić przeniesienie od poprawek.** Poprawki Sonara w tym samym commicie co przeniesienie utrudniają review (`git diff --color-moved` nie pokaże ich czysto). Najlepiej: commit 1 — czysty move (skrypt), commit 2 — poprawki. Przy merge **nie** squashować, żeby `git blame -C` dalej działał.
|
||
4. **Zakomentowany kod** (np. stary `ConverterSwitch` w `TrainCommandsElectrical.cpp`) został przerobiony z `//` na `/* … */`, a nie usunięty. Skoro i tak gaśniemy uwagi Sonara (S125), lepiej go usunąć.
|
||
5. **Nazewnictwo / folder** — opis mówi „TBD”. `vehicle/train/` jest OK; trzeba to tylko potwierdzić przed merge, bo późniejsza zmiana to kolejny duży konflikt.
|
||
6. **„Probably works™” / „Please test them as well”.** Testy jednostkowe nie linkują `TTrain`, więc regresje wyjdą tylko w grze. Proszę o krótką listę ręcznie przetestowanych komend (światła, pantografy, drzwi, hamulce, radio) albo o checklistę dla testerów.
|
||
7. `CMakeLists.txt` — sprawdzić, że nowe pliki są też w innych systemach budowania, jeśli jakieś są (np. projekt VS, skrypty `build-dispatch`).
|