Skip to content
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

Variant and VariantF prefixing helpers #2

Open
paluh opened this issue Jan 28, 2021 · 14 comments
Open

Variant and VariantF prefixing helpers #2

paluh opened this issue Jan 28, 2021 · 14 comments

Comments

@paluh
Copy link
Contributor

paluh commented Jan 28, 2021

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
Copy link
Owner

Sure!

@paluh
Copy link
Contributor Author

paluh commented Jan 29, 2021

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
Copy link
Owner

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

@paluh
Copy link
Contributor Author

paluh commented Feb 5, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

paluh commented Feb 8, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

paluh commented Feb 9, 2021

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
Copy link
Contributor Author

paluh commented Feb 9, 2021

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
Copy link
Contributor Author

paluh commented Feb 9, 2021

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
Copy link
Owner

dariooddenino commented Feb 10, 2021

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
Copy link
Contributor Author

paluh commented Feb 10, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

paluh commented Feb 10, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants