Page MenuHomeSealhub

Reverse single reference
ClosedPublic

Authored by kuba-orlik on Mar 25 2018, 20:32.
Referenced Files
Unknown Object (File)
Tue, May 7, 15:54
Unknown Object (File)
Wed, May 1, 00:32
Unknown Object (File)
Tue, Apr 30, 10:37
Unknown Object (File)
Thu, Apr 25, 19:27
Unknown Object (File)
Thu, Apr 25, 14:48
Unknown Object (File)
Fri, Apr 19, 19:46
Unknown Object (File)
Wed, Apr 17, 10:53
Unknown Object (File)
Tue, Apr 16, 03:18
Tokens
"Party Time" token, awarded by bartosz-gordon.

Details

Summary
  • refactor with_test_app so it allows to run tests without automatically starting the test app
  • add reverse_single_reference field type - Ref. T763
NOTE: Dla czytelności rozbiłem tego diffa na dwie iteracje, jedna to prosty refaktor with_test_app, a mięcho do review jest w drugiej iteracji - dla wygody polecam w panelu "history" porównywać Diff1 z Diff2:
Screenshot-2018-3-25 ⚙ D200 Reverse single reference.png (298×912 px, 22 KB)

Diff Detail

Repository
rS Sealious
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kuba-orlik created this revision.
  • Add reverse-single-reference
kuba-orlik changed the visibility from "All Users" to "Public (No Login Required)".Mar 25 2018, 21:26
kuba-orlik added a parent revision: Restricted Differential Revision.Mar 26 2018, 08:26

Rebase because d196 changed

arkadiusz-wieczorek added inline comments.
lib/app/app.js
113

Czy czystszym podjeściem nie będzie modyfikowanie tej flagi z poziomu dedykowanej funkcji?

lib/app/app.js
113

Co zyskalibyśmy w ten sposób?

lib/app/app.js
113

Czytałem o tym w książce "Czysty kod" ale nie pamiętam uzasadnienia, jak znajdę czas to poszukam

kuba-orlik added inline comments.
lib/app/app.js
113

Ok, jak poznam zalety takiego rozwiązania w tym przypadku to z chęcią wdrożę zmiany ;)

Jeżeli znajdziesz to uzasadnienie po wylądowaniu tego diffa, to zrób proszę komentarz w audycie do powiązanego z nim commita

kuba-orlik marked an inline comment as done.

rebase

bartosz-gordon subscribed.

Failuje poniższy test - przy imporcie with-test-app zapomniałeś zdestrukturyzować { with_running_app }.

failing.png (97×422 px, 6 KB)

lib/app/base-chips/field-types/reverse-single-reference.js
25

(question) Jest jakiś powód, dla którego używasz tutaj params.field_name, a poniżej samego field_name?

42

(should) Na wszelki wypadek można tu zmienić == na ===

80

(question) Czemu służy ta linia? :P

lib/app/metadata.js
6

(question) Czy czasem nie chciałeś użyć tutaj zmiennej COLLECTION_NAME, a nie stringa? :P

This revision now requires changes to proceed.Mar 26 2018, 22:17
piotr-ptaszynski subscribed.

Idzie request changes, ale kawał dobrej roboty!

lib/app/app.js
113

Wtrącę swoje 3 grosze, że rzeczywiście to co mówi Arkadiusz, to jest dobra praktyka - uniezależniamy się od reprezentacji stanu, natomiast w tym wypadku, jak to jest 7 przypisań w 2 plikach i jest to fajnie przesłonięte w testach, to na razie bym nie zmieniał. Jak się zacznie rozrastać, to rzeczywiście warto

lib/app/base-chips/collections/password-reset-intents.subtest.js
21

(question)

Wiem, że to przyszło w ramach innego taska, ale czy na pewno to jest dobra praktyka? Czy aplikacja powinna zdradzać, że dane konto nie istnieje?

lib/app/base-chips/field-types/reverse-single-reference.js
72

(should)
Może to opinionated, ale kocham redukcję nestingu (tak zresztą sam zrobiłeś w następnej funkcji) - sugeruję odwrócenie warunku w ifie, wtedy ten pojedynczy return powędruje na początek a reszta nie będzie musiała być w else

105

(could) Znowu jest miejsce na redukcję nestingu - jeżeli format_params[0] nie przyjmuje żadnej z wartości oraz może, dla spójności, zamiast return undefined zrobić return decoded_value.

111

(could)
Mimo wszystko parseInt bez drugiego argumentu napełnia mnie niepokojem ;)

lib/app/base-chips/field-types/reverse-single-reference.subtest.js
42

(could) A może const bs = await Promise.map(...) i wyleci for?

66

Nieznałem tego - fajne!

88

(should) Ten fragment:

await with_stopped_app(async ({ app, rest_api }) => {
     await create_referencing_collections(app, "with_reverse" && true);
     await app.start();
     await create_resources(app);})

powtarza się w każdym teście - moim zdaniem można to łatwo wyłączyć do osobnej funkcji przyjmującej parametr with_reverse

179

(question) Dlaczego 3?

test_utils/with-test-app.js
90–91

(could)

Skoro się powtarza w try i catch to dobry przypadek użycia sekcji finally

kuba-orlik marked 14 inline comments as done.
kuba-orlik added inline comments.
lib/app/base-chips/collections/password-reset-intents.subtest.js
21

Tak, bo aplikacja i tak by zdradzała ten fakt pozwalając lub nie założyć konto na dany adres ;)

lib/app/base-chips/field-types/reverse-single-reference.js
25

Tak. field_name to nazwa pola, do którego jest przypisany nasz reverse-single-reference. params.field_name to nazwa pola typu single-reference, które jest w jakiejś osobnej kolekcji.

Skoro nie jest to oczywiste, najwyraźniej niefortunnie dobrałem nazwy zmiennych. Pomyślę, jak by je zmienić.

80

Ups, cały dzień kodzenia padł mi na mózg :P Już usuwam

lib/app/metadata.js
6

ups :D

test_utils/with-test-app.js
90–91

O, nie wiedziałem, że js to ma :x

kuba-orlik marked 5 inline comments as done.
  • Review fixes

Jeżeli moje pytanie wynika z niezrozumienia to zachęcam do lądowania. A jeżeli nie, to zachęcam do poprawienia ;)

This revision is now accepted and ready to land.Mar 27 2018, 20:08

Jeżeli moje pytanie wynika z niezrozumienia to zachęcam do lądowania. A jeżeli nie, to zachęcam do poprawienia ;)

Najmocniej przepraszam, starałem się odpowiedzieć na każde z pytań, ale jakimś cudem ten jeden komentarz mi uleciał. Rozumiem, że chodzi o ten:

https://hub.sealcode.org/D200#inline-1343

odnosząc się do fragmentu:

results = await rest_api.get(
  "/api/v1/collections/B?filter[references_in_a][pairity]=odd"
);
assert.equal(results.length, 3);

Jeżeli to jest fragment o który Ci chodzi, to odpowiedź jest następująca:

w setupie testu tworzymy 6 zasobów w A - połowę z polem "pairity" na "odd", a połowę na "even". Każdy z nich wskazuje na jeden z 6ciu elementów kolekcji B. Dlatego jest 6/2 = 3 zasobów w B na które wskazuje jakiś A z "pairity" ustawione na "odd"

This revision was automatically updated to reflect the committed changes.