-
Notifications
You must be signed in to change notification settings - Fork 24
Add common PropName, AttrName, ClassName #30
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,30 @@ | ||||||||
module Web.HTML.Common where | ||||||||
|
||||||||
import Prelude | ||||||||
|
||||||||
import Data.Newtype (class Newtype) | ||||||||
|
||||||||
-- | A type-safe wrapper for property names. | ||||||||
-- | | ||||||||
-- | The phantom type `value` describes the type of value which this property | ||||||||
-- | requires. | ||||||||
newtype PropName value = PropName String | ||||||||
|
||||||||
derive instance newtypePropName :: Newtype (PropName value) _ | ||||||||
derive newtype instance eqPropName :: Eq (PropName value) | ||||||||
derive newtype instance ordPropName :: Ord (PropName value) | ||||||||
|
||||||||
-- | A type-safe wrapper for attribute names. | ||||||||
newtype AttrName = AttrName String | ||||||||
|
||||||||
derive instance newtypeAttrName :: Newtype AttrName _ | ||||||||
derive newtype instance eqAttrName :: Eq AttrName | ||||||||
derive newtype instance ordAttrName :: Ord AttrName | ||||||||
|
||||||||
-- | A wrapper for strings which are used as CSS classes. | ||||||||
newtype ClassName = ClassName String | ||||||||
|
||||||||
derive instance newtypeClassName :: Newtype ClassName _ | ||||||||
derive newtype instance eqClassName :: Eq ClassName | ||||||||
derive newtype instance ordClassName :: Ord ClassName | ||||||||
derive newtype instance semigroupClassName :: Semigroup ClassName | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still have a problem with this instance - I can see that argument for wanting to support If I have But I can also see why that might be unexpected for some people, which is kinda why I don't like the instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's a good point, and it's the reason why in my own applications I have a separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would still be law abiding if it included the space, it's just that I think either instance we choose has the potential to trip people up, depending on how they see the type ("it's just a newtype string wrapper" vs a thing with its own semantics). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, now I can't remember why I thought that was a semigroup law; I must be thinking of the left/right identity laws for monoid. For what it's worth, we could assert that this is a thing with its own semantics, where Regardless, if I'm overconfident and the camps are split then it's much better not to have an instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It arose here a while back: purescript-halogen/purescript-halogen#512 🙁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I say we take it out to avoid the confusion. Thanks for the link! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either instance is a perfectly fine semigroup; semigroups only need associativity, and the space-adding version’s associativity follows from normal string concatenation being associative. The difference is that if you use the space-adding Semigroup, you can’t have a Monoid instance because there’s no identity element. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Will do. For reference here is where the instance was added for Halogen purescript-halogen/purescript-halogen#451 |
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 not sure calling these "type safe" is exactly right, maybe just "semantic"?
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.
Happy to remove it. Just copying what already exists in Halogen https://github.com/purescript-halogen/purescript-halogen/blame/c8b50378b948fcba9e10d75da718fc7dae3a609f/src/Halogen/HTML/Core.purs#L171