Synpunkter på min uppdaterade CRouterBasic

  • Författare
  • Meddelande

desk15

html-guru

  • Inlägg: 68
  • Blev medlem: 03 sep 2015, 11:28

Synpunkter på min uppdaterade CRouterBasic

Inlägg11 maj 2016, 13:44

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
"Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the universe trying to produce bigger and better idiots. So far, the universe is winning." - Rick Cook

desk15

html-guru

  • Inlägg: 68
  • Blev medlem: 03 sep 2015, 11:28

Re: Synpunkter på min uppdaterade CRouterBasic

Inlägg11 maj 2016, 14:21

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?
"Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the universe trying to produce bigger and better idiots. So far, the universe is winning." - Rick Cook
Användarvisningsbild

mos

dbwebb

  • Inlägg: 10982
  • Blev medlem: 10 nov 2011, 09:52
  • Ort: Ronneby / Bankeryd

Re: Synpunkter på min uppdaterade CRouterBasic

Inlägg12 maj 2016, 09:14

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.
...
..:
.... /mos
Användarvisningsbild

mos

dbwebb

  • Inlägg: 10982
  • Blev medlem: 10 nov 2011, 09:52
  • Ort: Ronneby / Bankeryd

Re: Synpunkter på min uppdaterade CRouterBasic

Inlägg12 maj 2016, 10:07

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.
...
..:
.... /mos

desk15

html-guru

  • Inlägg: 68
  • Blev medlem: 03 sep 2015, 11:28

Re: Synpunkter på min uppdaterade CRouterBasic

Inlägg12 maj 2016, 13:22

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
"Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the universe trying to produce bigger and better idiots. So far, the universe is winning." - Rick Cook
Användarvisningsbild

mos

dbwebb

  • Inlägg: 10982
  • Blev medlem: 10 nov 2011, 09:52
  • Ort: Ronneby / Bankeryd

Re: Synpunkter på min uppdaterade CRouterBasic

Inlägg12 maj 2016, 14:17

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å.
...
..:
.... /mos

Vilka är online

Användare som besöker denna kategori: Inga registrerade användare och 18 gäster