Poprawienie UX Sealiousa - treści błędów spowodowanych niepoprawnymi deklaracjami

adrian-wydmanski · 31 October 2018 04:04

Hej,

Zauważyłem aktywny task dla Sealiousa: Poprawić UX new Sealious.App . Przyszedł mi pomysł poprawienia UX nie tylko dla Sealious.App, ale dla wszystkich metod, funkcji i klas używanych przez użytkownika. Dlatego napisałem coś takiego:

Kiedy user niepoprawnie zadeklaruje kolekcję, np.

const tasks = app.createChip(Sealious.Collection, {
    name: 123, // niepoprawna nazwa
    fields: [
        { name: "title", type: "text", required: true },
        { name: "done", type: "boolean", required: true },
        { name: "hack", type: "text"}
    ],
    access_strategy: { default: "owner", create: "logged_in" },
});

Sealious wyświetli błąd Error: Wrong Collection declaration format; expecting "name" field to be string, found: number

Albo dla takiej niepoprawnie zadeklarowanej kolekcji:

const tasks = app.createChip(Sealious.Collection, {
    name: "tasks", 
    fields: [
        1 // niepoprawny field_type
    ],
    access_strategy: { default: "owner", create: "logged_in" },
});

Wyświetli błąd: Error: Wrong Collection decalration format; expecting "fields" to be an array of objects, found: number

Ponieważ tego typu usprawienia UX wymagają dużo ifów, to chcę stworzyć osobną klasę obsługującą tego typu błędy, żeby nie zaśmiecać kodu.

Dajcie znać co myślicie na ten temat.

kuba-orlik · 31 October 2018 10:39

Hej! Słuszna uwaga, takie error messages znacznie ułatwiają samodzielne odkrywanie API i są kluczowe do dobrego UX frameworka Dzięki za inicjatywę!

Dotychczas powstrzymywaliśmy się z pracami nad UX tego API, ponieważ duża część z nich będzie przepisana w najbliższych miesiącach (od dłuższego czasu zmagam się z aktualizacją API strategii dostępu, field-types i kolekcje są następne w kolejce). Niemniej jednak jak sugerowane przez Ciebie zmiany przejdą proces review, to z radością domerge’uję je do Sealiousa!

Jak zwykle, mam “kilka” uwag technicznych:

nie możemy domerge’ować tego brancha z GitHuba, ponieważ repo na GH to tylko mirror naszego repo z Sealhuba - to na Sealhubie odbywa się development i wszystkie zmiany z GH będą nadpisane. Praca nad Sealiousem odbywa się wg Sealhub Workflow
korzystanie z if/else to zawsze dla mnie czerwona flaga - zwłasza, jeżeli są pozagnieżdżane. Jest trudno wywnioskować z nich intencję programisty i łatwo popełnić błędy przy wprowadzaniu do nich zmian. Zdecydowanie lepszym rozwiązaniem jest ubranie tego w jakąś deklaratywną strukturę (np. obiekt, klucz → pożądany typ). A może jest jakaś gotowa biblioteka, która jest odpowiedzią na problem walidacji obiektów? Czytelność jest bardzo ważna

adrian-wydmanski · 1 November 2018 03:18

Update:

Utworzyłem klasę DeclarationValidator, głowne idee:

validator - obiekt definujący poprawność chipa. Na chwile obecną istnieją 3 typy: collection, field_type i access_strategy_type, każdy jest w osobnym pliku.
w konstruktorze podajemy typ validatora, np. collection oraz deklarację chipu.
na instancji wywołujemy metodę is_declaration_valid(), która odpala dwie inne metody: validate_declaration_type() oraz validate_declaration_fields(), które sprawdzają poprawność odpowiednio typu deklaracji oraz pól, które ono zawiera.
metoda ta zwraca albo pusty string, albo wiadomość błędu
specjalnie nie wywołuję błędu wewnątrz klasy, ponieważ chcę dać użytkownikowi wybór

Używam tę klasę w metodzie createChip() w pliku app.js:

createChip(constructor, declaration) {
	const chip = new constructor(this, declaration);

	if( constructor.type_name === "collection" || constructor.type_name === "field_type" || constructor.type_name === "access_strategy_type") {
		const declaration_validator = new DeclarationValidator(constructor.type_name, declaration);
		const validator_error_msg = declaration_validator.is_declaration_valid();

		if( validator_error_msg.length !== 0) {
			throw new Error(validator_error_msg);
		}
	}

	this.ChipManager.add_chip(
		constructor.type_name,
		declaration.name,
		chip
	);
	return chip;
}

Przykładowy błąd:

adrian@adrian-XPS-15-9570:~/Documents/sealious-project$ node .
/home/adrian/Documents/sealious/lib/app/app.js:118
				throw new Error(validator_error_msg);
				^

Error: Wrong declaration format:
 - expected field 'name' to be 'string', found: 'number'
    at App.createChip (/home/adrian/Documents/sealious/lib/app/app.js:118:11)
    at load_base_chips (/home/adrian/Documents/sealious/lib/app/load-base-chips.js:97:7)
    at new App (/home/adrian/Documents/sealious/lib/app/app.js:66:3)
    at Object.<anonymous> (/home/adrian/Documents/sealious-project/index.js:19:13)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)
    at startup (bootstrap_node.js:204:16)
    at bootstrap_node.js:625:3

Planuję napisać osobną klasę errorów dla błędów UX. Nie wydaje mi się, żeby rzucenie stosu było przydatne w tym przypadku.
Oczywiście trzeba trochę dodać więcej typów validatora, rozwinąć też trzeba validator dla access_strategy_type, dużo różnych metod jest tam wykorzystywanych,

Dajcie znać co myślicie. Jeżeli mój pomysł zostanie przyjęty, napiszę stosowną dokumentację.

kuba-orlik · 2 November 2018 12:45

Stosy są przydatne, bo wtedy wiemy, w jakim pliku jest deklaracja, która sprawia kłopot

if( constructor.type_name === "collection" || constructor

^ taki kod nie przejdzie. Lepiej będzie przenieść walidację do konkretnych podklas (field-type, collection, etc) i tam dokonywać sprawdzania. Osobna klasa raczej nie będzie potrzebna. Czy szukałeś jakiejś biblioteki, która robiłaby walidację obiektu na podstawie deklaratywnego opisu?

Pamiętaj też, aby sformatować kod prettier-em ⚡ Prettier - formatter kodu

No i testy. Cokolwiek teraz dopisujemy, musi być otestowane.

adrian-wydmanski · 2 November 2018 15:53

Oczywiscie, ze nie przejdzie, bo to nie jest skonczony projekt tak jak napisalem, na razie validatory sa tylko dla wyzej wymienionych chipow. Chodzi mi o przedstawienie idei, w ktorym miejscu w kodzie widze ich walidowanie.

Czy z podanego przeze mnie stosu jestes w stanie wywnioskowac, ktory chip wywolal ten blad? Blad jest rzucony w createChip(), sam stos nie poda ci informacji, ktora deklaracja jakiego chipu jest bledna.

Dlaczego uwazasz, ze lepiej rozdrobnic walidacje na kazdy typ chipu, zamiast wyabstrahowac ten problem i umiescic rozwiazanie w osobnej klasie?

kuba-orlik · 2 November 2018 16:01

W stosie jest linijka:

   at Object.<anonymous> (/home/adrian/Documents/sealious-project/index.js:19:13)

Która doprowadzi mnie wprost do miejsca, (z dokładnością co do linijki/znaku!) w którym znajduje się błędna deklaracja Jestem jak najbardziej za tym, aby umieszczać w treści błędu dodatkowe informacje ułatwiające diagnozę. Jednocześnie uważam, że error stacki są mega przydatnym standardem i lepiej ich nie chować.

Bo wtedy logika odnośnie klas field-types, collections etc nie jest przemieszana - tylko trzymana w jednym miejscu. Dzięki temu jak przyjdzie czas na robienie jakichś zmian np. w field-type, to będzie to można zrobić w jednym miejscu

adrian-wydmanski · 2 November 2018 16:10

Fakt, tutaj sie zgodze. Aczkolwiek cala reszta stosu jest absolutnie w tym miejscu nie przydatna, a wrecz moim zdaniem przeszkadza w czytelnosci tego konkretnego rodzaju bledu, a przeciez po to zajmujemy sie UX.

Przemysle to.

Kazda deklaracja chipu powinna byc obiektem (za wyjatkiem kolekcji, ktora akceptuje tez string - nazwe istniejacej kolekcji), kazda poprawna deklaracja powinna miec tez pole name typu string. Mamy tutaj logike, ktora w twoim rozwiazaniu trzeba powielic dla kazdego chipu.

Deklaracja chipow collection, field-type oraz access-strategy-type nie zmienila sie od ~trzech lat, swoja droga. Jezeli jednak zaszla by potrzeba zmiany deklaracji, idziesz do folderu, ktory zawiera wszystkie definicje poprawnych deklaracji chipow i zmieniasz wedle uznania.

Chce napisac klase DeclarationValidator tak, aby byla jak najmniej podatna na zmiany formatu deklaracji.

Oczywiscie mozna rozdrobnic walidacje dla kazdego typu chipu, tylko… po co? Skoro mozna miec wszystko w jednym miejscu. Wiem, ze odszedles od idei Sealiousa jako “orurowania” dawno temu, ale w tym przypadku osobny, agnostyczny, niezalezny od samego frameworka plugin (nie wiem czy to poprawny termin, jezeli nie to wybacz) do walidacji, ktorego mozna latwo wymienic, ktory to co waliduje tez mozna latwo wymienic, wydaje mi sie byc bardzo dobrym rozwiazaniem.

kuba-orlik · 2 November 2018 16:28

A tu niespodzianka, bo szykują się duże zmiany Będziemy powoli odchodzić od chipów i chip-managera, będą zwykłe klasy i zwykłe require’y. Będą też istotne zmiany w API (jestem w trakcie refaktoryzacji access-strategy, wg lekcji nauczonych po tym eksperymencie).

Jestem za tym, aby znajdować wspólną warstwę abstrakcji dla wielu przypadków - ale:

 		if( constructor.type_name === "collection" || constructor.type_name === "field_type" || constructor.type_name === "access_strategy_type") {

to nie jest abstrakcja, to jest zbiór przypadków brzegowych. Obawiam się, że może być trudno rozwiązać ten problem w tym miejscu tak, aby faktycznie był abstrakcją.

W skrócie:

	abstrakcja	enkapsulacja
`constructor.type_name === "collection" constructor.type_name === "field_type" constructor.type_name === "access_strategy_type"`	NIE	NIE
sprawdzanie w każdym typie chipu osobno	NIE	TAK

Być może uda nam się mieć 2xTAK - zachęcam do eksperymentowania

adrian-wydmanski · 2 November 2018 17:32

kuba-orlik:

Jestem za tym, aby znajdować wspólną warstwę abstrakcji dla wielu przypadków - ale:
 		if( constructor.type_name === "collection" || constructor.type_name === "field_type" || constructor.type_name === "access_strategy_type") { 
to nie jest abstrakcja, to jest zbiór przypadków brzegowych. Obawiam się, że może być trudno rozwiązać ten problem w tym miejscu tak, aby faktycznie był abstrakcją.

Aj uczepiles sie tych ifow Powtarzam: to jest tymczasowe rozwiazanie, poniewaz moja klasa obsluguje na razie tylko 3 chipy. Po rozszerzeniu jej do wszystkich typow chipow, createChip() bedzie wygladac tak:

createChip(constructor, declaration) {
	const declaration_validator = new DeclarationValidator(constructor.type_name, declaration);
	const validator_error_msg = declaration_validator.is_declaration_valid();

	if( validator_error_msg.length !== 0) {
		throw new Error(validator_error_msg);
	}

	const chip = new constructor(this, declaration);
	
	this.ChipManager.add_chip(
		constructor.type_name,
		declaration.name,
		chip
	);
	return chip;
}

Ify wrzucilem tam tylko dlatego, ze wystarczylo mi czasu tylko do obsluzenia collection, field-type i access-strategy-typ.

Zauwaz ze konstruktor DeclarationValidator wyglada tak:

    constructor(validator_type, declaration) {
        this.declaration = declaration;
        this.validator = validators_map[validator_type];
    }

this.validator = validators_map[validator_type]; - ta linijka robi dokladnie to co lubisz najbardziej - zastapienie ifow obiektem (np. validator_type przyjmuje collection, a validators_map['collection'] zwraca definicje poprawnie zadeklarowanej kolekcji, wobec ktorej analizowana bedzie definicja kolekcji).

Okej, dobrze wiedziec. Czy planujesz odejsc w takim razie od deklaracji w stylu:

{
    name : "tasks",
    fields : [
        { ... },
    ]
}

?

kuba-orlik · 3 November 2018 08:34

Jak Ty mnie znasz

Takie rozwiązanie bez ifów mi się podoba - mam jeszcze tylko problem z tym, że opis składni danego typu chipu w tym podejściu nie jest trzymany w miejscu, w którym deklarowany jest ten typ chipu.

Myślę, że można pogodzić nasze podejścia w następujący sposób:

DeclarationValidator byłby zwykłą funkcją, a nie klasą;
przyjmuje jako argumenty opis składni + obiekt do sprawdzenia;
wywoływany jest w konstruktorze danego typu chipu;

Wtedy zadaniem danego typu chipu jest wiedzieć, jaką składnie mają jego deklaracje

Raczej nie - ale każda z następujących refaktoryzacji będzie stopniowo usuwała ideę chipów i app.createChip. Stąd między innymi moje parcie, aby zmiany robić poza tą funkcją

adrian-wydmanski · 3 November 2018 18:37

Taki był mój zamysł od samego początku

W takim razie do każdego katalogu zawierającego chipy dodam plik zawierający poprawną definicję deklaracji (np. lib/app/base-chips/field-types/field-type-validator.js)

Okej, nie mam z tym problemu.

Spoko.

Obecnie chipy nie mają konstuktorów, ty chcesz żeby walidacja była kompatybilna z nową wersją API. Ja chcę, żeby walidacja była niezależna od wersji API.

Domyślam się, że nawet w nowej wersji, gdzie chipy są po prostu klasami, gdzieś będzie pętla iterująca po nich wszystkich - właśnie tam chciałbym umieścić walidację deklaracji.

kuba-orlik · 4 November 2018 12:19

Jeżeli będą to duże obiekty, to jest to spoko pomysł. Jeżeli nie, to najlepiej będzie dać obiekty opisujące składnię po bezpośrednio jako argument do funkcji walidującej

Sorry, miałem na myśli nie konstruktory, ale te funkcje tworzące, takie jak np:

https://hub.sealcode.org/source/sealious/browse/alpha/lib/chip-types/field-type.js$16

(kod pełen ifów, wiem - ale to stary kod )

Planuję zrezygnować z globalnego store’a chipów, dlatego iteracja po klasach nie będzie potrzebna

adrian-wydmanski · 6 November 2018 01:39

Na to się nie zgadzam. Albo wsadzamy wszystko bezpośrednio jako argument, co jest chybionym pomysłem, ponieważ poprawna definicja deklaracji access_strategy_type jest długa, albo wszystko idzie do osobnego pliku, nawet jeżeli to są tylko 3 linijki.

Ja w miejsce tych ifów bym wsadził wywołanie funkcji walidującej, jeżeli zwróci ona błąd to go rzuć - myślisz o tym samym?

kuba-orlik · 6 November 2018 09:07

Trudno jest mi to zobaczyć oczami wyobraźni. Zrób tutaj, jak uważasz i zobaczymy, jak to wyjdzie

Tych akurat if-ów będzie trudno pozbyć się za pomocą funkcji walidującej, bo one jeszcze zależnie od przypadku robią różne skutki uboczne Więc walidacją tego nie zastąpimy

adrian-wydmanski · 13 November 2018 02:05

Okej, więc:

DaclarationValidator jest teraz funkcją, a nie klasą. Pozbyłem się również ifa z createChip. Na razie praca nad samą funkcją jest zakończona, czekam na nową wersję API.

Pytanie: jakie pola w deklaracji access-strategy-type są wymagane?

Oraz: Jak przez mgłę pamiętam jak mówiłeś, że is_proper_value w field_typach jest wymagane, ale nie ma go np. w deklaracji html. Jak to w końcu jest?

kuba-orlik · 21 November 2018 18:06

Sorry za opóźnienie, dopadło mnie grypsko/przeziębieńsko.

is_proper_value jest wymagane. html dziedziczy z text, dlatego nie musi podawać własnej implementacji tej funkcji

Co do kodu - wrzuć proszę diffa do sealhuba:

https://hub.sealcode.org/w/sealhub_workflow/audyt-i-review/review-workflow/

i tam będziemy mogli kontynuować wygodnie dyskusję na temat kodu

adrian-wydmanski · 26 November 2018 13:10

https://hub.sealcode.org/D423

kuba-orlik · 4 January 2019 15:26

Zrobiłem review Twojego Diffa

adrian-wydmanski · 20 January 2019 23:04

Ping
https://hub.sealcode.org/D423