-
Notifications
You must be signed in to change notification settings - Fork 36
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 unsafeTextWithLength #230
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! :)
I have a few comments.
There are also some CI failures that need fixes.
-- | identical to the strict version | ||
unsafeLazyTextWithLength :: Lazy.Text -> Int -> Doc ann | ||
unsafeLazyTextWithLength txt l = Text l (Lazy.toStrict txt) |
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.
What's the use case for this function? I don't imagine that wide characters and emojis will typically exist in the form of a lazy Text values?!
If there's no obvious use case, I'd prefer not to add this function.
-- | Convert text to Doc. Must not contain newlines. | ||
-- Useful when dealing with wide characters and emojis |
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.
The documentation should clarify why and how this function is useful for dealing with wide characters and emojis.
-- | Convert text to Doc. Must not contain newlines. | ||
-- Useful when dealing with wide characters and emojis | ||
unsafeTextWithLength :: Text -> Int -> Doc ann | ||
unsafeTextWithLength txt l = Text l txt |
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 think this definition is a bit misplaced between these Pretty
instances. I suggest placing it below unsafeTextWithoutNewlines
.
-- | Convert text to Doc. Must not contain newlines. | ||
-- Useful when dealing with wide characters and emojis | ||
unsafeTextWithLength :: Text -> Int -> Doc ann | ||
unsafeTextWithLength txt l = Text l txt |
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.
The Text
invariants need to be handled in some way too:
prettyprinter/prettyprinter/src/Prettyprinter/Internal.hs
Lines 153 to 159 in 0dbe08f
-- | Invariants: at least two characters long, does not contain '\n'. For | |
-- empty documents, there is @Empty@; for singleton documents, there is | |
-- @Char@; newlines should be replaced by e.g. @Line@. | |
-- | |
-- Since the frequently used 'T.length' of 'Text' is /O(length)/, we cache | |
-- it in this constructor. | |
| Text !Int !Text |
I think that invariant should be handled by user because single character may have different length. |
Add
unsafeTextWithLength
according to #103