Page MenuHomeSealhub

Reverse single reference
ClosedPublic

Authored by kuba-orlik on Mar 25 2018, 20:32.
Referenced Files
F969841: D200.id692.diff
Fri, Nov 22, 01:26
F969836: D200.id666.diff
Fri, Nov 22, 01:17
F969796: D200.diff
Thu, Nov 21, 20:56
Unknown Object (File)
Wed, Nov 20, 17:09
Unknown Object (File)
Tue, Nov 19, 15:53
Unknown Object (File)
Tue, Nov 19, 15:53
Unknown Object (File)
Tue, Nov 19, 15:53
Unknown Object (File)
Tue, Nov 19, 15:53
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
Branch
reverse-reference (branched from alpha)
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 480
Build 480: arc lint + arc unit

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
106 ↗(On Diff #674)

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

lib/app/app.js
106 ↗(On Diff #674)

Co zyskalibyśmy w ten sposób?

lib/app/app.js
106 ↗(On Diff #674)

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
106 ↗(On Diff #674)

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
24 ↗(On Diff #682)

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

41 ↗(On Diff #682)

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

79 ↗(On Diff #682)

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

lib/app/metadata.js
5 ↗(On Diff #682)

(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
106 ↗(On Diff #674)

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
71 ↗(On Diff #682)

(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

104 ↗(On Diff #682)

(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.

110 ↗(On Diff #682)

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

lib/app/base-chips/field-types/reverse-single-reference.subtest.js
41 ↗(On Diff #682)

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

65 ↗(On Diff #682)

Nieznałem tego - fajne!

87 ↗(On Diff #682)

(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

178 ↗(On Diff #682)

(question) Dlaczego 3?

test_utils/with-test-app.js
87

(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
24 ↗(On Diff #682)

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ć.

79 ↗(On Diff #682)

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

lib/app/metadata.js
5 ↗(On Diff #682)

ups :D

test_utils/with-test-app.js
87

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.