Sida 1 av 1

Synpunkter på min uppdaterade CRouterBasic

InläggPostat: 11 maj 2016, 13:44
av desk15
Pull Request
Länka det här för att få fler synpunkter på min ändring i CRouterBasic från Anax-MVC om det är en bra idé eller inte.

Mvh
Dennis

Re: Synpunkter på min uppdaterade CRouterBasic

InläggPostat: 11 maj 2016, 14:21
av desk15
Nu kom jag på att när man använder default parametrar så funkar det inte.
Kan jag bara ändra min pull request om Mos gör så att jag ska ändra något eller kan jag göra det själv?

Re: Synpunkter på min uppdaterade CRouterBasic

InläggPostat: 12 maj 2016, 09:14
av mos
När man gjort en pullreq så kan man fortfarande ändra sin egen kod och göra en ny pull req så tar den hänsyn till de senaste ändringarna.

Re: Synpunkter på min uppdaterade CRouterBasic

InläggPostat: 12 maj 2016, 10:07
av mos
Det blev en ambitiös pull req. Iallafall känns det så när det kommer en stor klump. För att göra det enkelt för den som underhåller ett projekt så är det bättre med flera mindre pull reqs som är tydliga och gör specifika ändringar. De är enkla att merga in för man ser direkt vad de gör för ändringar.

Låt oss fixa det först, sen kan vi se mer i detalj på inplementationen av CIndexController.

Vad vi behöver göra är att dela upp din pull req i delar. Det ser ut som det räcker att dela in den i två delar. En del för småfixar och en del för CIndexController. Flera små pull req bättre än en stor.

Det enklaste är nog om du skapar en feature branch som ligger till grund för varje pull req. I princip gör du så här:

1. Hämta hem min senaste version av Anax MVC och lägg i en feature branch.
2. Cherry pick de commits du vill skall ligga i branchen.
3. Skapa en pull req baserat på din feature branch.

Upprepa för nästa pull request.

Lyckas du med detta så är du extra cool.

Det står lite om det här:
http://stackoverflow.com/questions/5256 ... est-commit

Jag tycker att stöd för en CIndexController kan vara bra att ha. Det kan komplettera default routen "*".

Men som du säger behöver vi vara säkra på om det finns påverkan på kompabilitet. Men det ser vi enklare om vi har en renare pull req. Kanske kan vi lägga till några unittestfall också, men det kan vi ta sen. Det du säger om parametrar kan kanske behöva några testfall.

Re: Synpunkter på min uppdaterade CRouterBasic

InläggPostat: 12 maj 2016, 13:22
av desk15
För varje dag så gillar jag git mer och mer. Cherry-pick var väldigt smidig.

Iallafall, nu har jag delat upp det i tre delar. En som fixat buggar, en som förbättrar koden och den sista som uppdaterar CRouterBasic.
Jag har också lagt till ett testfall för CDispatcherBasic som testar parameter validator.

Hoppas det blir mycket enklare nu.

Mvh
Dennis

Re: Synpunkter på min uppdaterade CRouterBasic

InläggPostat: 12 maj 2016, 14:17
av mos
Yep, blev mycket enklare.

Dessutom blev det enklare att se den "felaktiga" patchen som annars troligen bara hade smugit med. Där kan man tänka sig att enhetstester borde ha varnat för den. Där ser vi en bra sak om man har sina enhetstester.

Två mergade och en kvar, den som kräver ett litet extra öga. Routerfixen är en sådan man gärna vill dra ned lokalt och dubbelkolla innan man mergar.

Stabilt jobbat.

Ska kika på routerfixen också.