michie1 / bc

Script that reads all orders of a topic from wtos.nl, puts everything in the cart of bike-components.de and generates a Google spreadsheet with Python, lxml, Google API, Docker
5 stars 5 forks source link

Type add_cart and fetch product data outside add_product #29

Closed michie1 closed 4 years ago

bcvanmeurs commented 4 years ago

Ik heb het getest: types en tests waren ok, maar ik kwam een kleine bug tegen, #30. Het werkt verder naar behoren, hij loopt nu ook niet meer vast op jouw hoi voorbeeldje.

michie1 commented 4 years ago

Wat er staat ziet er allemaal logisch uit en ik kan het volgen.

Mooi!

Het opsplitsen van die functies lijkt me handig voor testen inderdaad.

Jep.

Maar wat verwacht je precies van een review? Moet ik het ook de code een keer runnen en testen? > Want dat heb ik nog niet gedaan.

Even doornemen en zien dat er wat veranderd gaat worden. Runnen hoeft niet per se, tenzij je denkt dat er bugs geïntroduceerd zijn.

michie1 commented 4 years ago

Ik heb het getest: types en tests waren ok, maar ik kwam een kleine bug tegen, #30.

Ik heb nog een checkbox toegevoegd om automatisch types en tests te runnen voor iedere commit. Ik reageer op de bug in #30.

Het werkt verder naar behoren, hij loopt nu ook niet meer vast op jouw hoi voorbeeldje.

Ja, dat was de bedoeling ook. Ik wilde even kijken of het script nog verder ging als er 1 item fout gaat.

bcvanmeurs commented 4 years ago

Ik heb nog een checkbox toegevoegd om automatisch types en tests te runnen voor iedere commit.

Ik begrijp niet precies wat je bedoelt en waar ik dat terug zou moeten zien.

bcvanmeurs commented 4 years ago

Hierop door zoekend vond ik ook dat mypy ook werkt met pre-commit: https://github.com/pre-commit/mirrors-mypy Maar pytest niet: https://github.com/pre-commit/pre-commit-hooks/issues/291

michie1 commented 4 years ago

Ik heb nog een checkbox toegevoegd om automatisch types en tests te runnen voor iedere commit.

Ik begrijp niet precies wat je bedoelt en waar ik dat terug zou moeten zien.

Screenshot from 2019-12-08 14-26-54 In #13

michie1 commented 4 years ago

Hierop door zoekend vond ik ook dat mypy ook werkt met pre-commit: https://github.com/pre-commit/mirrors-mypy Maar pytest niet: pre-commit/pre-commit-hooks#291

Dat hoeft van mij ook niet. Ik vind het prima dat je commits maakt en dat de types dan nog falen. Zolang dat maar niet gebeurt als je naar master merged.

bcvanmeurs commented 4 years ago

Check, duidelijk!