-
Notifications
You must be signed in to change notification settings - Fork 73
Non-empty strings & various operations #100
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
Conversation
src/Data/String/NonEmpty.purs
Outdated
derive newtype instance semigroupNonEmptyString ∷ Semigroup NonEmptyString | ||
|
||
instance showNonEmptyString :: Show NonEmptyString where | ||
show (NonEmptyString s) = "(NonEmptyString.fromString " <> show s <> ")" |
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.
fromString
returns Maybe NonEmptyString
so here I guess should be unsafeFromString
or something like that.
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.
Hmm, yeah I guess so. I didn't want to add that originally as the unsafeFromString
I had in mind was really unsafe, but I guess if we do it as fromJust <<< ...
and give it the Partial
constraint that'll work well enough.
btw where do you think this should leave? class IsSymbolNonEmpty sym where
safeStr :: SProxy sym -> NonEmptyString
instance isSymbolNonEmpty :: (IsSymbol s, Equals s "" False) => IsSymbolNonEmpty s where
safeStr _ = asNonEmpty $ reflectSymbol (SProxy :: SProxy s)
where
asNonEmpty :: String -> NonEmptyString
asNonEmpty = unsafeCoerce |
For now, not in here. The compiler solved classes are going to be moved around a bit in 0.12, so after that we can consider including it in here perhaps :) |
Can I just say pre-emptively, I'm not a big fan of the name "safe", since it implies possibly-empty strings / String types which are inhabited by an empty string are "unsafe", which I don't think really makes sense as a claim; often you do want or even need to include the empty string in your consideration. It's just that having more specific types is sometimes (not always) more appropriate. I think a more appropriate name would be |
Yeah, that sounds perfectly reasonable. Any thoughts on the rest of this @hdgarrood? (No need to review fully if you don't have the time) |
I've only looked over it briefly but it all looks good to me 👍 |
Kinda depends on #99, since I continued after that.
Alternative to #98 that keeps the underlying representation as just being a plain
String
, and adds functions and tests for all the usual stuff.