Refactor for better composability with custom types in sites#30
Refactor for better composability with custom types in sites#30
Conversation
src/Ginger/Resource.elm
Outdated
| , fromJsonWithEdges | ||
| , fromJsonWithoutEdges | ||
| , categoryListFromJson | ||
| , CategoryList, ResourceDataConstructor, depictionsOfClass, edgeFromJson, firstDepictionOfClass, resourceDataPipeline |
There was a problem hiding this comment.
Formatting lijkt wat raar hier
| @docs getCategories | ||
| @docs getDepiction | ||
| @docs getDepictions | ||
| @docs firstDepictionOfClass |
There was a problem hiding this comment.
Misschien toch mediaclass noemen ipv van class? Ik denk bij class aan een HTML class. Wel een goede rename verder.
|
|
||
| So you'll see this used in function signatures like: | ||
| type alias Resource = | ||
| SiteData (ResourceData Edges) |
There was a problem hiding this comment.
Ik raakte een beetje in de war omdat zowel ResourceData en SiteData extensible records zijn. Is ResourceData alleen extensible omdat er evt edges in kunnen zitten? Zouden we dan niet gewoon een maybe (of andere type) kunnen gebruiken?
edit: ah wacht dat staat erboven, zodat we compile time weten weten of er edges zijn. Zit ook wel weer wat in
There was a problem hiding this comment.
Inderdaad, met een Maybe weet je het niet at compile time. Weet niet helemaal zeker hoe vaak we dat echt gebruiken, maar aangezien het eerst ook zo was heb ik het maar behouden.
| , publicationDate : Maybe Time.Posix | ||
| , media : Media | ||
| , blocks : List Block | ||
| , properties : Decode.Value |
There was a problem hiding this comment.
Waarom heb je gekozen om properties op ginger niveau te houden?
There was a problem hiding this comment.
Ik had ze eerst weggehaald, maar toen kwam ik in de knoop met de functies in Ginger.Resource.Extra. In principe zouden we wel aparte types moeten maken voor de verschillende soorten resources (zoals een Event), zodat we de properties die bijvoorbeeld bij een event horen bij het decoden al eruit halen, maar het probleem is dat je ze dan na het decoden kwijt bent. Dan moet je in de Cache module van de website extra dictionaries gaan maken voor die specifieke resources, want anders kan je een Event niet meer heropbouwen uit de cache.
Je zou hetzelfde probleem met blocks hebben, maar omdat die volgens mij wel door de hele site hetzelfde is kunnen we gewoon het bestaande dictionary in de cache aanpassen en hoeven we geen extra dictionaries toe te voegen.
| {-| -} | ||
| type Status | ||
| = Authenticated Identity (Maybe (ResourceWith Edges)) | ||
| = Authenticated Identity (Maybe Decode.Value) |
There was a problem hiding this comment.
Waarom heb je hier gekozen omdat decode.value mee te geven ipv een type parameter van resource?
There was a problem hiding this comment.
Ik dacht eigenlijk dat het te veel impact zou hebben om daar een type variabele aan toe te voegen, maar nu ik er nog een keer naar kijk valt het eigenlijk wel mee. Ik was eerder aan de Data.Status aan het denken die ook door Fetch gebruikt wordt denk ik.
Zal ik er dan maar een type parameter van maken?
|
Misschien is het trouwens wel een goed idee om eerst uit te proberen hoe het is om mod_equilar hierop aan te passen voordat we het mergen, voor het geval we nog problemen tegen komen. Details{ Voordat het werkt moet je misschien wel even je |
2 participants
Based on #29, so that needs to be merged first.
When all review comments are fixed, we should add a commit to bump the package version.