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

Use render to implement an instance for Show #13

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

L7R7
Copy link
Contributor

@L7R7 L7R7 commented Aug 15, 2020

This is a first, naive solution for #12. (less naive than just putting an orphan instance in Burrito.Internal.Render).

Baiscally I just moved the two functions in Burrito.Internal.Render that had a dependency to Burrito.Internal.Type.Template over to Burrito.Internal.Type.Template and fixed compile errors. The tests are green, and show does what I would expect.

I'm not entirely convinced that this is a good solution. In terms of cohesion, the functions I moved should reside in Render. I just moved them over to avoid the cyclic dependency.

What do you think about this?

Copy link
Owner

@tfausak tfausak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great!

All else being equal I would definitely prefer to keep all the stuff related to rendering in the render module. But I think this approach is the least of all evils. It's definitely better than orphan instances or .hs-boot files 😄

@@ -28,7 +28,6 @@
-- In short, use @parse@ to parse templates and @expand@ to render them.
module Burrito
( Parse.parse
, Render.render
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could keep exporting this as a type-restricted version of show.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think I got that. Could you explain that a little? What is the advantage of having render in addition of show?

Apart from that, I would just have to add render to the exports of Burrito.Internal.Type.Template to get what we want, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One advantage is API stability. Since Burrito.render currently exists, I would like to keep it there unless there's a compelling reason to remove it.

In general I like to have monomorphic versions of polymorphic functions since they can sometimes help with type inference. In this particular case it doesn't matter much since Burrito doesn't expose any polymorphic ways to create templates. But imagine if it implemented a Read instance; then you could use render . read to round-trip a template. The alternative would be something like show @Template . read or show . (\ t -> t :: Template) . read.

Copy link
Contributor Author

@L7R7 L7R7 Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thank you for the explanation! Should be done in the latest commit
Edit I'll fix the unnecessary whitespace changes as soon as I'm back at my laptop

src/lib/Burrito/Internal/Type/Template.hs Outdated Show resolved Hide resolved
src/lib/Burrito/Internal/Type/Template.hs Outdated Show resolved Hide resolved
L7R7 and others added 2 commits August 16, 2020 10:01
Co-authored-by: Taylor Fausak <taylor@fausak.me>
Co-authored-by: Taylor Fausak <taylor@fausak.me>
@L7R7
Copy link
Contributor Author

L7R7 commented Aug 16, 2020

I don't even know what .hs-boot files are 😅

Apart from the open suggestion with the export for render, is there anything you'd like to see in this PR?

This keeps `Burrito.render` in the public API
Copy link
Owner

@tfausak tfausak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! This should make playing around with templates in GHCi a much nicer experience. Thank you!

One tiny little change: Could you keep the export list in the same order as before? The order in the code is the order that things show up in the documentation. I like showing parse, then render, then expand.

@tfausak tfausak merged commit 7dfdf71 into tfausak:master Aug 18, 2020
@tfausak
Copy link
Owner

tfausak commented Aug 18, 2020

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants