-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add Empty inline #298
Add Empty inline #298
Conversation
src/ast_inline.ml
Outdated
| Empty | ||
| Concat of 'attr * 'attr inline list |
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.
I'm slightly unsure what we gain by introducing the Empty
data constructor here. If we want a unit value, that would already be available as Concat ([], [])
, and we could add, e.g., a named value as a shorthand for this: let empty attr = Concat (attr, [])
Meanwhile, adding a new data constructor will require additional cases during analysis that don't actually improve the logic afaict.
Do you perhaps have any examples in mind where an explicit Empty
data constructor is helps clarify things that couldn't be addressed through my proposed empty
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.
Sorry, I should have explained where this comes from.
The initial use case came from the need being able to have a link with an empty label (of type inline), filled later with CSS.
I fully agree using a shorthand, as you propose, is a better solution, as it avoids having two ways to represent the same thing (Empty and Concat ([], []). I had overlooked that.
I'll update the PR with your proposal.
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.
Looks great. Thanks!
CHANGES: - Add an `empty` inline value (ocaml-community/omd#298 @cuihtlauac) - Remove stdcompat dependency (ocaml-community/omd#304 ocaml-community/omd#305 @hannesm, review by @samoht) - Minimum supported OCaml version is now 4.13 (ocaml-community/omd#304 @hannesm)
CHANGES: - Add an `empty` inline value (ocaml-community/omd#298 @cuihtlauac) - Remove stdcompat dependency (ocaml-community/omd#304 ocaml-community/omd#305 @hannesm, review by @samoht) - Minimum supported OCaml version is now 4.13 (ocaml-community/omd#304 @hannesm)
Fix #297