search

dariooddenino / purescript-record-prefix

A blazing fast library to add a prefix to a record's labels
7 stars 1 forks source link

`Variant` and `VariantF` prefixing helpers #2

Open paluh opened 3 years ago

paluh commented 3 years ago

Would you like prefixing helpers for Variant and VariantF to this library? I have working prototype for Variant so I can provide a PR.

dariooddenino commented 3 years ago

Sure!

paluh commented 3 years ago

After a discussion on purescript-beginners channel it seems that this can be implemented by coercing into VariantRep and back (unvariant is not an option and this is not a folding like in the case of the Record). Could you please tell me if you think that this can be acceptable implementation?

dariooddenino commented 3 years ago

I'm not very familiar with VariantRep, but I don't think it should be a problem.

paluh commented 3 years ago

I was talking bullshit. Prefixing Variant is also a fold (as suggested by Nate).

I've this prototype (not ready for a PR) but I wonder if this polymorphic row in the result is a real problem:

https://github.com/paluh/purescript-record-prefix/blob/e8f1daf35cfdeb1cbae377163a8effb10e8735ec/src/Data/Variant/Prefix.purs#L50

I think that we should get "closed row" back. What is your opinion?

dariooddenino commented 3 years ago

Why should the polymorphic row be a problem? Do you have a case where this breaks? At a quick glance it looks pretty similar to the record solution

paluh commented 3 years ago

Why should the polymorphic row be a problem? Do you have a case where this breaks?

I think that "opening" a Row of a Variant / Record can be seen as problem (the input row are closed) because we don't have an easy way to apply another generic transformation to it - like in this example we need a type annotation:

https://github.com/paluh/purescript-record-prefix/blob/master/src/Data/Variant/Prefix.purs#L50

dariooddenino commented 3 years ago

Mm right, this is pretty annoying. I just checked and it's not the case for the record implementation which has a an additional Row.Lacks constraint. I'm not sure if this would help, but have you tried using it?

paluh commented 3 years ago

I had Lacks constraint in my initial version. It doesn't change the output unfortunately... It seems that I have to calculate the output Row type and inject values into the final variant. I'm afraid that it is going to complicate the implementation a bit ;-)

paluh commented 3 years ago

I could do this with typelevel-eval easily... but I'm not sure if we want to bring such a dependency here. Could you please tell me what do you think?

paluh commented 3 years ago

So this is a version which uses typeleve-eval to precompute resulting row type:

https://github.com/paluh/purescript-record-prefix/blob/6a7d303b6b9d9385a833012f06d6771fead0bf67/src/Data/Variant/Prefix.purs#L79

As you can see it doesn't require intermediate annotations but is a bit more complicated - we use here folding on the type level:

ToRow <<< FoldrWithIndex (UnprefixStep pre) NilExpr <<< FromRow

I should probably make clear distinction between these two folds strategies (one purely on type level using typlevel-eval and second on the value level using heterogeneous) used by the module by commenting them properly.

dariooddenino commented 3 years ago

I think it's a good approach. Despite what's written in the readme, this library never had any pretence of being fast or efficient (which could be done via FFI I guess), so I don't see any problem in adding an extra dependency.

I wonder If this library should change name after this merge

paluh commented 3 years ago

I think it's a good approach.

Cool! I'm going to rebase all these commits so they form a single one so I can provide a nice PR.

Despite what's written in the readme, this library never had any pretence of being fast or efficient (which could be done via FFI I guess), so I don't see any problem in adding an extra dependency.

I don't think that this typelevel computation is going to impact runtime performance. I'm not sure how JS looks like exactly regarding the hfoldWithIndex but it folds into a single Variant.case_ # Variant.on ... # Variant.on .... expression so I think it will be OK ;-)

I wonder If this library should change name after this merge

prefix-rows sounds too "type-level" I think... prefix alone sounds to abstract... "Naming is the hardest problem..." :-P

dariooddenino commented 3 years ago

Great and thank you! :) Could you also please add a couple of simple tests and expand the readme?

prefix-row is too "type-level", but is also the only correct one. Maybe something like `label-prefix"

paluh commented 3 years ago

Could you also please add a couple of simple tests and expand the readme?

Yeah, sure. I want to add support for VariantF, add tests and extend the README as well. We can also try make the README a part of the test suite like it is done in the case of the purescript-cookbook or in my other libs (like here or here). Could you please tell me what do you think about this idea?

Maybe something like `label-prefix"

label-prefix or prefix-label both sounds good I think.