-
Notifications
You must be signed in to change notification settings - Fork 106
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
code review #35
Comments
I don't know, I am usually against these kinds of corporate coding. It is way too much overhead to the work actually done. I think for this small project that nobody uses so far, it is pointless. |
It should create almost no overhead compared to the way we should do things now. You can work as fast and dirty as you wish, if you do that in a separate branch. When you are merging into master, you should polish your code and discuss important changes and design decisions before merging. Using a code-review tool would just make the last stage easier and more organized than a GitHub PR single-thread discussion. |
Do you have experience with such tools? Reviewable has a terrible corporate website telling me that a senior PHP developer thinks it is a miraculous tools. This is an evident sign, we should not use it. I am not sure if we need such a thing, but if yes I would prefer something open-source, e.g. barkeep? |
We need GitHub integration (otherwise there is no point in this) so if you want open-source, we can use Both seem to have the same features, so I don't really care which one we choose, but I think we should use one. |
uvědomuješ si, že na tom budem dělat jen my dva a že já netušim, co vlastně děláš ty a ty zas tvrdíš, že nechápeš, co dělá ten zbytek, takže je to celý zbytečný? Code review je od toho, aby se do hotovýho projektu na kterým dělá velkej tým a kterej běží někde na produkci, nedostaly nový bugy. Tohle neni ani hotovej projekt, ani velkej tým, ani produkční prostředí. |
to je stejná blbost jako psát všechny issues anglicky, přestože jsme jediný lidi, co to tady čtou. |
Ještě to čte Martin Popel ;-) |
|
Ad 3: Pochybuju, že by review těchto dvou issues pomohlo. V jednom případě byla chyba někde jinde, než změny, takže kdyby to někdo po mě četl, tak by mu to taky nedošlo a v druhym případě těžko říct, jestli by reviewera napadlo, že to funguje jen s absolutníma cestama. Každopádně k odhalování zmíněných bugů neni review ale testy, takže příklady, kterýs dal, nejsou validní. Ad 4: wontfix & close Ad 5: Nemám s tim takovou zkušenost. Moje zkušenosti jsou takový, že přes ty reviews i tak projde velká část bugů a že ve výsledku to zpomaluje postup víc, než to pomáhá. Ad 6: Doufám, že tyhle naše blitky nebudou tou dobou jediná dokumentace toho, co bude hotový. Navíc se tu podle mě řeší spíš blbosti než něco, na co by se někdo někdy ptal. |
Ad 5: zpomaluje to postup z větví do masteru, který má být pomalý, takže to je jedině dobře. |
Píšu, že ve výsledku to škodí víc než pomáhá. Rozhodně nepíšu, že to zpomaluje postup z větví do masteru, píšu že to zpomaluje postup celkově. |
A já s tím nesouhlasím, protože si myslím, že postup se má odehrávat ve větvých (takže na něm se nic nezpomaluje) a code review se děje před mergem z větve zpátky do masteru, což rozhodně nemá být unáhlená akce, takže tam je zpomalení naopak žádané. Tvůj celkový postup to krátkodobě může zpomalit jedině tím, že budeš muset číst něco, co napsal někdo jiný, aby to mohl mergovat. Ale tohle drobné zdržení se Ti bohatě vrátí na tom, že se do masteru nedostanou věci, které by Tě později zdržely mnohem víc. |
Rozumim. Já dál ale tvrdim, že se tam ty věci dostanou tak jako tak. |
Přinejmenším by se tam nedostaly některé nesrozumitelné věci, které tam teď jsou; že je něco napsané nesrozumitelně, na to se při code-review určitě přijde. Nicméně shodneme se na tom, že pokud na tom v nejbližší době budeme pracovat jen dva, tak je zbytečné o tom dál diskutovat. |
saving model with iteration number for session1,2,3 + sync session2-3
There are many code review tools integrated with GitHub (eg. Reviewable). Should we use one of them in our workflow?
The text was updated successfully, but these errors were encountered: