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

Add unsafeTextWithLength #230

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add unsafeTextWithLength #230

wants to merge 3 commits into from

Conversation

reixn
Copy link

@reixn reixn commented Jul 19, 2022

Add unsafeTextWithLength according to #103

Copy link
Collaborator

@sjakobi sjakobi left a 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.

Comment on lines 463 to 465
-- | identical to the strict version
unsafeLazyTextWithLength :: Lazy.Text -> Int -> Doc ann
unsafeLazyTextWithLength txt l = Text l (Lazy.toStrict txt)
Copy link
Collaborator

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.

Comment on lines 458 to 459
-- | Convert text to Doc. Must not contain newlines.
-- Useful when dealing with wide characters and emojis
Copy link
Collaborator

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.

Comment on lines 458 to 461
-- | 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
Copy link
Collaborator

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
Copy link
Collaborator

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:

-- | 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

@reixn
Copy link
Author

reixn commented Feb 14, 2023

I think that invariant should be handled by user because single character may have different length.

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.

2 participants