Skip to content
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

Regal tidy #11

Closed
wants to merge 18 commits into from
Closed

Regal tidy #11

wants to merge 18 commits into from

Conversation

jablkoj
Copy link
Member

@jablkoj jablkoj commented Aug 1, 2013

Vyrobene modely pre:
ludi, skoly, adresy..
sutaze, rocniky, kola..
zadania, body

prisposobenie admina

slovenske nazvy modelov a fieldov

…problems(pre ulohy, body..), contest(pre sutaze, rocniky, kola..)
…prisposobene adminske rozhranie:

(ze ake stlpce sa vypisuju a podobne)
na urlku regal/gen. Tato funkcia vygeneruje hromadu ludi, adries, sutazi, kol..
Aby si ich clovek nemusel vyrabat sam po pripadnom precisteni databazy.

K tomu samozrejme patria prislusne viewy a template pre dashboard.
… Persons, ktorou je abecedny filter.

Vo viewoch (v People/admin.py) je tato ficura uz davnejsie.
…uca sa tlacitka Pridaj objekt.

V pripade ze mame napriklad zobrazene rocniky len KSP(nech jeho id je 2) tak sa do url tlacidla prida ?competition=2.
Nasledne add-formular pre rocniky moze predvyplnit polozky ako competition alebo number.

(Pythonie zdrojaky tejto ficury su v contest/admin.py, vacsina uz bola nakodena davnejsie)
@koniiiik koniiiik mentioned this pull request Aug 1, 2013
@koniiiik
Copy link
Member

koniiiik commented Aug 2, 2013

Oveľa lepšie, ale stále to nie je úplne ono. Chalani, PEP8. Sú tam strašne dlhé riadky (to by sa ešte dalo zniesť), ale aj whitespace okolo čiarok, dvojbodiek, zátvoriek a operátorov dávate úplne náhodne a dosť blbo sa to potom číta.

Inak namiesto používania u'string' odporúčam radšej na začiatok každého súboru drbnúť

from __future__ import unicode_literals

Takisto, namiesto __unicode__ skúste __str__python_2_unicode_compatible.

Tým pádom to bude forwards kompatidebilnejšie s Py3K, keď raz na také budeme prechádzať. (Možno by bolo dokonca fajn to tak nasadiť od začiatku.)

Teda ďalší bod, skúste to na Pythone 3.2 alebo 3.3.

Zatiaľ toľkoto ku všeobecným veciam, postupne popridávam inline commenty. (Dokonca som sa kvôli vám prinútil zapnúť Chrome, aby som ich vedel pridať, lebo Github, samozrejme, serie na browsery. d-: )

@koniiiik
Copy link
Member

koniiiik commented Aug 2, 2013

Ešte jedna vec, čo som videl all over the place. Jeden príklad za všetky:

str(self.number)+u'. ročník '+self.competition.__unicode__()

Formátovanie stringov sa takto nerobí. Všetky hentaké výrazy by som poprosil nahradiť niečím takýmto:

'%d. ročník %s' % (self.number, self.competition)

alebo alternatívne, ak sa vám viac páči nový štýl formátovania, tak

'{!d}. ročník {!s}'.format(self.number, self.competition)

Ideálne sa však rozhodnite pre jeden štýl formátovania a ten používajte všade. (Ja osobne uprednostňujem ten prvý, lebo ten druhý má pomerne zložitú syntax a nedarí sa mi zapamätať si ju. Predpokladám, že keďže väčšina z vás má background v C/C++, tiež vám bude viac sedieť.)

@koniiiik
Copy link
Member

koniiiik commented Aug 2, 2013

Jedného dňa mi budete musieť vysvetliť tie properties, čím je to také výnimočné a zložité, že na to treba tri rôzne podtriedy QuerySetu a vlastného Managera a nestačí jednoducho jeden tabular inline v adminovi.

@koniiiik
Copy link
Member

koniiiik commented Aug 2, 2013

Keď už som rozbehnutý v review ragi, máte veľkú pochvalu za existenciu testov. To je veľmi dobre, len tak ďalej a ešte viac. (-;

Na teraz končím, vrátim sa k tomu niekedy neskôr.

@mhozza
Copy link
Member

mhozza commented Aug 5, 2013

Ja len pre istotu - veci ktoré vám Koník alebo ja sprdol zmeňte v tejto branchi.
A aj odomňa máte pochvalu za testy.
Myslím, že keď to pofixujete, tak to môžme potom mergnuť.

@AnotherKamila
Copy link
Member

Otazka (asi na sysla): preco tam mame failujuce testy? Nechce s tym niekto niekedy nieco robit?

@jablkoj
Copy link
Member Author

jablkoj commented Aug 6, 2013

Kedze sme mysleli, ze nemame sahat do tohoto branchu, tak sa niektore z veci, ktore ste nam sprdli zmenili v branchi premenovanie. Hlavne sa tam urobilo formatovanie na PEP8.

Co podla mna znamena, ze ak nechceme mergeovat rucne,
tak zrejme musime aj vsetky ostatne opravy robit z tejto branche.

@mhozza
Copy link
Member

mhozza commented Aug 6, 2013

Ok, tak hadam v tej branchi nie su prilis velke zmeny navyse od reviewu (sudim podla nazvu branche), tak spravte rebase, tak aby to bolo v tejto branchi.

Takze este raz pre poriadok opravy podla review comentov PATRIA do tej istej branche. Nova funkcionalita NEPATRI do tej istej branche.

@koniiiik
Copy link
Member

koniiiik commented Aug 6, 2013

Ja súsim objasniť, prečo som sa vyjadril za novú branch v tomto prípade – pretože išlo o zmeny, ktoré síce nie sú nejak myšlienkovo extrémne náročné, ale vyžadujú prepísanie prakticky celej code base. Preto mi pripadá lepšie mať v mastrovi rovno tú novú, vyčistenú verziu a nie aj jej predchodcu.

@usamec
Copy link
Contributor

usamec commented Aug 6, 2013

Par veci k modelom:
Year je uplne nanic, ako som vam hovoril osobne (kedze tazko sa potom vysvetluje, ze na zimu je vysledkovka zo serii 1,2 a na leto zo serii 3,4). Radsej to nahradte niecim, ci zdruzuje vsetky serie v danom semestri. Osobne ako nazvy navrhujem: Seminar pre terajsiu Competion, Competion/Contest pre upraveny Year.

Potrebujete este v modeloch vyriesit nazivanie serii, t.j. z cisla 2 a Contestu, treba raz vyprodukovat "2. seria zimnej casti" a raz letnej (a pri OI nic). Mozno by pri kompetion nebol odvedzi flag, semester s 3 values (zima, leto, cely rok).

@koniiiik
Copy link
Member

koniiiik commented Aug 6, 2013

Potrebujete este v modeloch vyriesit nazivanie serii, t.j. z cisla 2 a Contestu, treba raz vyprodukovat "2. seria zimnej casti" a raz letnej (a pri OI nic). Mozno by pri kompetion nebol odvedzi flag, semester s 3 values (zima, leto, cely rok).

…alebo to rovno spraviť celé ako string. Snažiť sa hardcodovať kadejaké automatické kombinátory názvov nie je nutne dobrý nápad, lebo tak či tak tam raz bude niekto raz chcieť napchať názov, ktorý to nevie vygenerovať automaticky.

ananasj and others added 2 commits August 7, 2013 13:36
formatovanie stringov, komentare, zmeny nazvov (alphabet), render_to_response() nahradene
@mhozza
Copy link
Member

mhozza commented Aug 13, 2013

Ok, takze aky je stav? Preco sa tento pullrequest taha uz 2 tyzdne, a stale nevyzera ze by sa blizil k mergnutiu?

Kamilin mail ste asi citali.

Takze moj navrh je takyto:
co keby sme tento pullrequest rozdelili na viac mensich?
Prvy kus by mohol koncit modelmi.

Takze skuste spravit toto - urobit novu branch z modeloveho commitu, dostante ju do mregnutelneho stavu, spravte pullrequest, niekto z nas sa na to pozre a ak bude vsetko ok, tak sa to mergne.

Potom budeme nejak pokracovat.
@jablkoj @ananasj @syslo - niekto z vas by si to mohol zobrat na starosti a spracit to tak radovo do 24 hodin, nech sa to zacne hybat.

@mhozza
Copy link
Member

mhozza commented Aug 13, 2013

Druha moznost na toto tu sa vykaslat a spravit to odznovu poriadne pod Kamilinim vedenim - mozno to tak bude lepsie

@koniiiik
Copy link
Member

Heh, som tu mal rozpísaný taký komentár, ale som sa nechal odlákať. d-: Basically, súhlasím s týmto plánom. S tým, že spraviť odznova neznamená, že sa nedajú použiť kusy kódu, ktoré už existujú.

Mám pocit, že doteraz dosť veľký problém bol ten, že ste to tu robili celé tak jedno cez druhé, bez nejakého plánu, čo treba spraviť skôr a čo potom. Asi by nezaškodilo ujasniť si toto.

@AnotherKamila
Copy link
Member

No, tak ja zistujem, ze odzajtra som v BA, takze ak chcete, mozme sa
stretnut -- som k dispozicii cely den.

(Ja ale mam jeden podstatny problem: Moj plan bol, ze taketo
spindavacie+planovacie stretko malo byt uz to minule, ale ja mam na to
prilis malo sebavedomia :D (Napisat henten mail mi trvalo pol dna :D) Takze
ma treba trochu kopat, ak chcete, aby som sa efektivne papekom ohanala.)

Kamila

2013/8/13 Michal Petrucha notifications@github.com

Heh, som tu mal rozpísaný taký komentár, ale som sa nechal odlákať. d-:
Basically, súhlasím s týmto plánom. S tým, že spraviť odznova neznamená, že
sa nedajú použiť kusy kódu, ktoré už existujú.

Mám pocit, že doteraz dosť veľký problém bol ten, že ste to tu robili celé
tak jedno cez druhé, bez nejakého plánu, čo treba spraviť skôr a čo potom.
Asi by nezaškodilo ujasniť si toto.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11#issuecomment-22596796
.

@jablkoj
Copy link
Member Author

jablkoj commented Aug 14, 2013

A aby toho nebolo malo, tak v nedelu ideme (jablkoj, ananasj, syslo) na LTT.

A presvedcit troch veducich LTT ze tyzden pred nim maju nejaky volny cas na stretko a kodenie bude tazke :)

A kedze na LTT tiez nema zmysel robit nieco neLTTckove (a aj internet tam bude zufaly) tak si myslim,
ze nasa cast kodu sa minimalne do 1.9. odlozi do chladnicky (a necha ticho plesniviet)

@AnotherKamila
Copy link
Member

Je to asi OT a viem, ze ste na LTT, ale ja budem potom 2 tyzdne bez netu, tak pisem teraz sem.

IMHO by bolo dobre tento pull request ako celok urvat a radsej ho rozdelit na viac manageovatelne chunky (typu pridal som tieto dva-tri modely). To umozni nieco mergnut skor (co je dobry napad), a je to vseobecne podstatne prijemnejsie reviewovat par ucelenych riadkov ako neviemkolkoriadkove monstra. Hlavne by sa mi pacilo, keby lubovolna magia mala vlastny pull request: deklaracie jednoduchych modelov s par fieldami su v pohode viacere naraz, ale veci typu Syslove properties si imho zasluhuju vlastny pull request :D

Takze porozbijat, spravit nove pull requesty a tie sa pekne zreviewuju a casom hadam aj pomerguju :D

@koniiiik
Copy link
Member

Stotožňujem sa s Kamiliným nápadom, preto tento PR uzatváram.

@koniiiik koniiiik closed this Aug 31, 2013
@mhozza mhozza deleted the regalTidy branch June 12, 2016 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants