Diskussion:Developer guidelines

@André Costa (WMSE), lägg till en rad om hur licens lämpligen läggs till (licens-fil, headers, ...).

Bra tanke.

Så kort :

• Add a LICENSE-file to the root of the repo. Somthing about how they should/shouldn't look
• For python: Consider adding the following to a comment in the top of the file. (C) <Name>, <YEAR>. Depending on the license this can be followed by the GPL paste or something like Distributed under the terms of the <license> license.

• For php: Add the following to a comment in the top of the file. @license <license>

Den första/tredje är en must men den andra är optional?

Ett stycke om Licenser finns nu och frågan ägs av https://phabricator.wikimedia.org/T234596

We've been de facto using gist to share ad hoc data extracts and code snippets (André, Alicia). Let's make sure to cover it when reviewing the guidelines.

Definitely. By comparison I believe Sebastian puts his (here).

Jag har testat att använda Numpy style och det verkar funka bra. Dokumentation går att generera med Sphinx. Se Developer guidelines/Python documentation för mer info och instruktioner. Kanske kan vi använda det som standard?

I've stored my repos in Dropbox out of ignorance. Now I have the following situation:

I got a Dropbox "conflict copy" of my main script file in the repo ("create_infotexts.py (Konfliktkopia för Mattiass MacBook Air 2017-04-05")

I deleted the conflict copy (since I'm somewhat a cowboy)

On Github I have a branch `places_from_desc`.

If I do `git pull origin places_from_desc` the branch is not pulled to my local. I still see this for `git branch`.

Can the file conflict have created this non-existing relation to the actual branch on github?

~/Dropbox/wmse/SMVK/SMVK-Cypern_2017-01[master ?*]$ git branch

warning: ignoring ref with broken name refs/heads/master (Konfliktkopia för Mattiass MacBook Air 2017-04-05)

  depicted_people_logic

  enrich_description

  image_class

* master

  output_to_json_blob

Honestly, when I get into problems like these, I just burn the local files with fire and download a clean copy from Github.

BUT I still use Dropbox since it syncs the files I don't track on git, like reports, logs and notes, which I probably shouldn't be keeping in the relevant dirs anyway... But it's nice to have access to them.

Ah. I shouldn't have used git pull since that is actually both fetching and merging the branch into master (where I am standing). I should have used just git fetch origin places_from_desc.

To fix I run git reflog show to see the pretty git names of each event and select the HEAD(a){1}

I then ran git reset HEAD(a){1}

Did this actually undo the merge? Help wanted! See result here:

https://phabricator.wikimedia.org/M212

To get a local copy of the remote branch i then ran:

git fetch origin

git checkout --track origin/places_from_description

Now I can work on the branch again locally. Nothing do do with storing the repo i Dropbox obviously.

https://blog.github.com/2018-06-04-github-microsoft/

Utan att vara för alarmistisk kan det vara lägligt att kolla vad det finns för några alternativ.

Det kunde vara värt att kolla på om det finns något bra verktyg för att testa komplexitet automatiskt. Jag har ingen förhoppning om att det ska fånga allt perfekt, men det skulle kunna vara bra som ett steg innan manuell granskning. Stötte t.ex. på Code Climate som gör bl.a. detta och är "Free for open-source.".

Jag har använt Code Climate för vissa projekt (hostade på Github). Det jag tyckte att den var väldigt bra på var att flagga nästan-duplicerad kod vilket ger en indikation på var refaktorering kan vara bra. Jag vill minnas att det var lite strul med att köra över default-inställningarna men annars är det inte ett dåligt verktyg att ha i lådan. Går även att integrera med webhooks i github så att man får feedback innan man mergar en PR.

Ett exempel https://codeclimate.com/github/lokal-profil/mediawiki-extensions-Wikispeech/code

Kollade som hastigast och det ser ut som att en del saker där kan vara användbara. Främst tycker jag att varningar för duplicering (som du nämner ovan) och cognitive complexity ser intressanta ut. Hårda gränser för antalet rader i funktioner vet jag inte om de är lika användbara, men kan funka som varningsflaggor.

Jo jag reagerade också på att långa filer/funktioner får så pass stränga straff.

Ska bli intressant hur exemplet ovan påverkas av refactor-js patchen.

Jag får inte "Trends" att fungera. Det är bara en laddningssnurra, oavsett vilken typ jag väljer.

Då är det nog något i din webbläsare:

• https://screenshots.firefox.com/z6zHo0GylGe6dxIA/codeclimate.com
• https://screenshots.firefox.com/ALPWjJvBp0e2noqA/codeclimate.com
• https://screenshots.firefox.com/8euAd8TFGeAC27GD/codeclimate.com

OBS skärmbilderna raderas automatiskt 2018-04-27

När man håller på att jonglera olika commits (t.ex. vid knepigare fall av rebasning) kan det vara bra att spara de med vettiga namn, så att man inte behöver skriva upp hashar. Detta går att göra med git tag NAMN, källa: https://stackoverflow.com/questions/134882/undoing-a-git-rebase#comment18027176_135614. Detta funkar för saker som git reset --hard, vilket nog ska gå via reflog också, men då krävs det lite detektivarbete.

Ett stycke borde läggas till om att man bör köra Toolforge jobb över grid.

Jobb via cron konverteras automatiskt men ej manuellt startade jobb. Utöver de instruktioner som finns i wikitech:Help:Toolforge/Grid så kräver det lite extra för att använda virtual environments samt se till att miljövariabler (som encoding) fungerar som de ska.

Se även phab:T177578

Ett stycke om följande bör vid tillfälle läggas till:

För att följa Wikimedia Privacy Policy bör vi använda oss av tools-static.wmflabs.org för att leverera javascript snarare än att använda externa cdn:er.

Jag har uppdaterat offentligkonst.se samt credit-my-cc men vi har nog kvar saker i andra verktyg.

För att kunna lägga till en reviewr måste dessa först ha adderats som collaborators och accepterat.

Använda WMSE-gruppen kanske skulle underlätta detta?

Även hur man resolvar conflicts i en PR (GitHub) som uppstår när en annan PR har mergats. (Svengelska galore här).

Lös dessa lokalt genom följande steg :

1. Check out branch containing conflicts (git checkout <branch>)
2. Pull master into branch (git pull origin master)
3. Resolve conflicts, git add fixed files then git commit
4. Push new version to Github (git push)

I gerrit, eller innan man pushat till GitHub kan man använda sig av git rebase.

Använder man sig av GitHub's inbyggda conflict resolver så misstänker jag att man får strul om man sedan vill lägga till fler comits i branchen. Därav ovanstående.

Efter två diskussioner idag har jag skissat upp hela förloppet från Task till Merge på GitHub. Ta en titt och fundera på om någon del behövs läggas till i den generella beskrivningen.

Schematic flow for a coding task (on GitHub):

1. Create a Phabricator Task for the task/bug
2. Create and check out a branch from master (git checkout -b <branch_name>) give the branch a short but descriptive name
3. Code in the branch (try to limit the changes to just things related to the task), commit when ready
4. Push the commit to Github (git push)
5. Create a Pull Request (PR)
   • Link from the PR to the Phabricator task (Task: <url> on last line)
   • Link to PR from the task
6. Go on coding and committing/pushing in the branch until you are ready for a review
7. Request review
   • Add reviewer to Github PR
   • Add #Patch-for-review to Task
8. Reviewer adds review and review-cycle starts
   • Code some more
   • Review again
9. Changes approved, ready for merge (I would suggest by the original coder)
   1. Squash-and-merge (make sure the new message contains both the description and the task link)
   2. Delete merged branch (on Github)
   3. Checkout master (locally) and pull changes (git checkout master; git pull)
   4. Delete branch locally (git branch -d <branch_name> or git branch -D <branch_name>)
   5. Add link to merge commit to Task and resolve Task (or move to done and remove #patch-for-review)

Man kan byta plats på punkt 5 och 6, lite beroende på hur man jobbar. D.v.s. om man gör lokala commits och bara pushar (och möjligen squashar) när det är dags för review.

Några potentiella problem:

Steg 2. `git checkout -b <branch_name` för att skapa och checka ut till den nya branchen?

Steg 4. Om en enbart gör en "git push" i steg 4 så pushar den de facto potentiellt i en annan branch! Istället bör en enligt denna tutorial lägga till följande:

`git push origin <branch_name>`

steg 2:

Done

steg 4:

Tror inte att detta behövs. Per default så går git push till branchen med samma namn på origin. Om en sådan branch inte finns så hojtar git till med en liten kodsnutt som du kan klipp-klistra för att att i samma veva skapa branchen. Dock måste du stå i branchen när du kör push (och <code>origin</code> måste självfallet vara länkat till github repot)

Not1: På Mattias dator kommer detta inte upp som ett förslag. Är möjligen https vs. ssh som spökar. Undersök, om inte så lägg in hela git-anropet i beskrivningen.

Not2: Den strängen som git föreslår är git push --set-upstream origin <branch_name>

Note: It's possible to disable certain types of merges (Settings → Options → Merge button). If you only allow squash-and-merge, it eliminates the risk of accidentally selecting another one.

Bör läggas till som en kommentar på någon övergripande rubrik om "setting up repo on Github" då tillsammans med kommentaren om "collaborators"

En not till: fetmarkera gärna att man verkligen måste stå i master när man skapar en ny branch, annars kan det bli konstigheter (man får med sig commits från den branch man befinner sig i som inte mergats till master).

Done