New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor for better composability with custom types in sites #30
base: master
Are you sure you want to change the base?
Conversation
src/Ginger/Resource.elm
Outdated
, fromJsonWithEdges | ||
, fromJsonWithoutEdges | ||
, categoryListFromJson | ||
, CategoryList, ResourceDataConstructor, depictionsOfClass, edgeFromJson, firstDepictionOfClass, resourceDataPipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting lijkt wat raar hier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
||
|
||
# Access data | ||
|
||
@docs getCategory | ||
@docs getCategories | ||
@docs getDepiction | ||
@docs getDepictions | ||
@docs firstDepictionOfClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misschien toch mediaclass noemen ipv van class? Ik denk bij class aan een HTML class. Wel een goede rename verder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mee eens. Fixed!
|
||
So you'll see this used in function signatures like: | ||
type alias Resource = | ||
SiteData (ResourceData Edges) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waarom heb je gekozen om properties op ginger niveau te houden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -53,7 +52,7 @@ import Time | |||
|
|||
{-| -} | |||
type Status | |||
= Authenticated Identity (Maybe (ResourceWith Edges)) | |||
= Authenticated Identity (Maybe Decode.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waarom heb je hier gekozen omdat decode.value mee te geven ipv een type parameter van resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.