Closed erlandsona closed 2 years ago
Should this additional rule live under this package name? It might be a bit unfindable? (Unless the repo becomes a go-to for codegen rules 😁 )
@Janiczek Sorry, we only discussed this privately: The final solution is me publishing generate-elm with everything you could want (field helper functions, lenses, prisms, conversions, svg, html, ... lots of stuff) regarding code-gen. (It's currently under construction and the public gh repo is way behind what's actually been built)
This PR which I'll merge into record-field-lens is only supposed to be a temporary solution to not pollute packages.
Ah... and as you see, I'll have to do some cleaning-up, restoring the package state to what was last published, as it seems like I've made some breaking changes that were pushed before I started elm-generate :shrug: (if anyone is willing to do it before me, go ahead 👀)
@lue-bird
(if anyone is willing to do it before me, go ahead 👀) Just pushed an update, not sure that was the only thing but should work?
~There are still a couple of things, names of tests etc. different.. Working from the previous published tagged version would I help I assume~ Should work now
~1 error: Test CheckPrismsWork
uses makeOneToN
but imports makeOneToN_
~
Corrected to makeOneToN_
names:
type
unions / variant types?@lue-bird
names:
- constructors → variants?
- custom types →
type
unions / variant types?
Not a fan of Variant coming from ReasonML... it's not necessarily clearer just cause it's different... also variant is the description of a type not it's constructors. In Reason you'd still have a "Variant Type w/ Constructors" 🤷♂️
Elm people usually refer to Sum types as "Custom Types" in my experience. But the pattern has so many synonyms I think we're screwed no matter where we end up.
Also Elm has tagged union
's not unions
(aka: structural typing) which would be type IntyString = String | Int
which is not expressible in Elm.
In TS to get proper tagged unions (aka: nominal typing) like in Elm you have to
type NominalThingy<A> = {type: "Just", data: A } | {type: "Nothing"}
// with constructors
const just = (data: A): NominalThingy<A> => ({type: "Just", data})
const nothing: NominalThingy<A> = {type: "Nothing"}
IIRC
They're called "constructors" because they're functions Just : a -> Maybe a
that "construct" a "type". Or at least that's how I read all this stuff 🤷
Correct, union type would be too broad, as elm only has tagged unions. Constructor (and custom type) feel like too broad terms as well. One is just a function that creates a thing, (the other sounds just like a non-core type)
variant is the description of a type not it's constructors
Yes and no. elm calls the possible tagged possibilities of a type
variants, see the terms on the guide:
Another benefit of this approach is that each variant can have different associated data
And I like naming the whole thing a variant/choice type as that's what it is
I'm not fixated, though, if you strongly disagree. Custom type is fine, as the official guide calls it that, too
Nah re the guide you're super right 😅 ugh that's mildly annoying lol... But totally down to change to Variant & Choice? That's new to but hey I'm down. Yeh change as you see fit. Long as the tests pass should work just fine...
Also if you figure out how to account for the imports that'd be neat too!
~Already implemented in my code-gen-AST version at generate-elm,
so I won't reimplement some ad-hoc import mechanism now.~
I misunderstood. You just want to add an import line? If NoMissingRecordFieldLens
already supports that, copy it over or create a shared helper
Also, I haven't completely looked through the code... But is the prefixing necessary/should it be more configurable? ... Don't mind me, it's probably fine as it is... ?
By prefixing you mean the c_
thing, yeh unfortunately... I put a line in the module doc about it. It's unavoidable. Can't use capital letters or as the first letter of a function name so "c" for constructor (change it to "v" for variant of you prefer that naming convention) and "" because Haskell's lens lib uses underscore for prisms like _Just
& _Left
... And because the underscore just felt more spacious 😅
Should prisms be generated for variants with 0 values attached? (in other words: Is making checks like is variantErr
reason enough?)
Also, I will rewrite the rule to generate only prisms that are actually used – just like NoMissingRecordFieldLens
Also, because we can't generate prisms in module
s of package dependencies, I will use the hierarchy Result.Variant.Ok
, creating a separate module for the helpers.
Special case which I might not implement: generate prisms for a module
internal type
RE: should it (prefix) be more configurable? Didn't even cross my mind but that's a great idea!
RE: Should prisms be generated for variants with 0 values attached? Not really a good justification for that I think because lenses/prisms are for accessing nested data... Think of it like trying to access something inside Unit? Does that make sense? If it does I can't really think of a use case for it.
RE: Also, because we can't generate prisms in modules of package dependencies, I will use the hierarchy Result.Variant.Ok, creating a separate module for the helpers.
Personally, I hate how much bloat there is in import
statements in Elm already...
There's 2 solutions here... One is update elm-accessors with prisms for all the types in Base. Maybe is already covered but I started working on like JSON? Didn't think to add result but those are a few could be added that way they don't need codegen and they'll get dead code eliminated if they're not used.
Two, personally I'd prefer they get generated into a single leaf file so I can import prisms generated for a custom type for a library (let's say Tree)
At work we codegen all our Lenses into an
import Gen.Lens as Lens
I wouldn't mind adding a
import Gen.Prism as Prism
Tho 🤔
Grouping variant prisms into one module
is a good idea for package type
s, it might not be for application type
s where it leads to import
cycles if the module
s containing the type
use it themselves and leads to a structure which is less nice.
Personally, I also like the visual distinction Stack.Variant.topDown
vs Variant.stack_TopDown
What exactly do you dislike this much about longer import
sections? I basically never find myself looking at them. Is missing IDE support the reason or is it somth. else?
(Unrelated: For both solutions, I like that prisms are always created locally just like lenses so that every user can use its own lens lib and isn't bound to package changes etc.)
RE: package types, which is what you mentioned when you said Result. I'm assuming stuff in Base is "package" which is why I suggested I'll just write those prisms and expose them from elm-accessors. But for library's that don't already expose prisms for their constructors (well one reason may be that the Constructors are opaque) but otherwise I wouldn't want to pollute the module namespace just for the sake of continuity with the library... It's not that "lot's of imports" bothers me in practice (well it sort of does...) It's that I think most people don't have "code folding" as a basic part of their editor workflow so while sure you can just scroll down past the imports (and I do) it is somewhat annoying that the first thing I see when I open up a >1000 LOC module that the first page of text is just imports 😅
Prisms have to be generated next to the type their operating over because we don't have "extensible variants" like OCaml or ReasonML have. We generate Lenses all in the same module so we can re-use the same "name" lens for a Person, User, Admin, Group, record. Which means our lens file grows sub-linearly with the number of field names in the project (which is great cause it means our codegen never gets that big like it can in C#)
With Prisms there's a much more targeted use case, being like Route Parser or JSON manipulation or something and you can have two sun types with the same constructors in scope in the same module so generating Prisms inline with the type definition is ideal.
Also just added ok
& err
Prisms to elm-accessors.
https://package.elm-lang.org/packages/erlandsona/elm-accessors/latest/Accessors#ok
Not sure if I should name it with the codegen prefix or not given it's mapping to stuff available in elm/core? 🤔
I'd prefix those that aren't implicitly imported (which Result(..)
is).
So your editor doesn't save and restore the view range of a file on exit? Is that a common thing / will that bother many people?
Also, I wouldn't call it polluting if you put it in the generated filed in source-directories
like source-generated
.
What I will admit: It's annoying to require users to create a new module
for every module
with type
s that have variant prisms you're using... → created an issue in elm-review
.
If you don't have a solution in mind for the import
structure problem, I'd still go finish implementing the Package.Variant.name
route (expect completion of the rule + config + tests etc. in about 3 days)
I'd prefix those that aren't implicitly imported (which Result(..) is). Cool, we're on the same page then. So leaving prefixes off
ok
&err
.So your editor doesn't save and restore the view range of a file on exit? I use vim, and I'm sure I could configure that but it's not something I've looked into? Should prolly just look into it tho 😅
Package.Variant.name route (expect completion of the rule + config + tests etc. in about 3 days) I don't usually set deadlines / expectations on open source work 😅
That said, Package.Variant.name
I still think should just be
src/Gen/Prism.elm
module Gen.Prism exposing (c_Variant1_A, c_Variant1_B, c_Variant2_A, c_Variant2_B)
import Package exposing (Type1(Variant1_A, Variant1_B), Type2(Variant2_A, Variant2_B)
Then at the call sites instead of
import Package.Variant1 exposing (c_Variant1_A, c_Variant1_B)
import Package.Variant2 exposing (c_Variant2_A, c_Variant2_B)
You'd just have
import Gen.Lens as Lens
import Gen.Prism as Prism
-- then later you couldj
get (Lens.remoteAddresses << Prism.c_GotData << at 2 << Lens.zip) -- : Maybe Int
That's just how I'm seeing things but do what you will 👍
I think we misunderstood each other: Not every type
but every module
will receive its own Module.Variant.name
prisms:
module Package.Variant exposing (a, b, aOther, bOther)
import Package exposing (Type(A, B), TypeOther(AOther, BOther))
Then at the call sites
import Package.Variant -- no exposing
import Field
get (Field.remoteAddresses << Package.Variant.gotData << at 2 << Lens.zip) -- : Maybe Int
And the ~deadline~ time approximation is more about setting expectations :dart:
And I think regardless, I love what we're doing here so lookin' forward to seein' whatever you come up with! 🤘
I changed generation module
names
-RemoteData.Variant.success
+RemoteData.On.success
like onEach
. Thoughts?
How about onJust
, onOk
, onErr
naming in your package?
Rule code, readme, preview configs, ... are now pushed to github. Some bugs still need to be found, tho :)
I kinda like on[Variant]
a lot actually... 🤘
If you have any examples of using those variant prisms in practice, go ahead and add them to the readme through a PR
Some updates
module
s are checked for variant type
simport
aliases that are recognized as usage, are added on automatic import
s and can be configured to either
module
name import Data.Extra.Local as Data
module
name with a suffix import Data.Extra.Local as DataX
onSucceed
→ variant Succeed
Take all of that and configure a rule like
VariantPrism.GenerateUsed.accessors
|> VariantPrism.GenerateUsed.inVariantOriginModuleDotSuffix "Extra.Local"
|> VariantPrism.GenerateUsed.importGenerationModuleAsOriginModule
|> VariantPrism.GenerateUsed.rule
elm-review generates on
-- [generated] import Data.Extra.Local as DataX
someUsage = DataX.onSuccess |or| Data.Extra.Local.onSuccess
If you have any examples of using those variant prisms in practice, go ahead and add them to the readme through a PR
@lue-bird I built this generator to be able to work with elm-url-codec without n+1 name generation like from @Janiczek's fork of this polluting. This is specific to people already bought in on this approach tho which is why @Janiczek's version of this makes more sense for more people.
Example:
myCodec =
Url.Codec.succeed CommentPage (is onComment) -- is : Lens -> Bool from erlandsona extra accessors
|> Url.Codec.s "post"
|> Url.Codec.string (get (onComment << Lens.pageSlug)) -- `get` + generated onVariant << regular lens
|> Url.Codec.s "page"
|> Url.Codec.int (get (onComment << Lens.pageNumber)) -- same as above
I see. I'm a bit surprised @Janiczek hasn't gone for the safer elm-codec
/ elm-serialize
/ elm-value
/ ... approach:
Url.Codec.variantUnion
(\commentPageDecode ... page ->
case page of
CommentPage commentPage ->
commentPage |> commentPageDecode
Page... ->
yellowEncoder f
)
|> Url.Codec.variant CommentPage
(\commentVariant ->
commentVariant
|> Url.Codec.s "post"
|> Url.Codec.string .pageSlug
|> Url.Codec.s "page"
|> Url.Codec.int .pageNumber
)
|> Url.Codec.variant Comment... (\... -> ...)
|> Url.Codec.variantUnionComplete
or at least
Url.Codec.succeed CommentPage (Page.onCommentPage |> get)
|> Url.Codec.s "post"
|> Url.Codec.string .pageSlug
|> Url.Codec.s "page"
|> Url.Codec.int .pageNumber
but I haven't looked deeper into the package to know if that's even possible with urls, so :shrug:
Rather seems like an anti-example of how prisms cover up bad API decisions (if that's the case here)
…Constructors of a Sum type.