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

Introduce strict-text and posn-strict-text wrappers and monadUserState-strict-text #240

Merged
merged 10 commits into from Jun 20, 2023

Conversation

alexbiehl
Copy link
Member

@alexbiehl alexbiehl commented Jun 12, 2023

This PR introduces the strict-text and posn-strict-text wrappers to allows working with Data.Text out of the box.

Implementation wise we could do a bit better by using Data.Text.Foreign.takeWord8 and avoiding the intermediate list of bytes in a codepoint but this will do for now.

@alexbiehl
Copy link
Member Author

alexbiehl commented Jun 12, 2023

Hm, tests do pass locally, not sure what's causing the issue.

@alexbiehl alexbiehl force-pushed the alex/strict-text branch 2 times, most recently from b72f69c to 939b605 Compare June 12, 2023 09:28
@andreasabel
Copy link
Member

andreasabel commented Jun 12, 2023

(As you already noticed) text is only a GHC-shipped package from 8.4 on, compare the release notes:

Adding a dependency like text might require to cabalize the testsuite first. (It's Makefile design is ancient, probably predating cabal.)

@alexbiehl
Copy link
Member Author

@andreasabel It looks like to fully cabalize the test suite is a long term effort, how about I disable the text specific tests on old GHCs?

@alexbiehl alexbiehl changed the title Introduce strict-text and posn-strict-text wrappers Introduce strict-text and posn-strict-text wrappers and monadUserState-strict-text Jun 17, 2023
@andreasabel
Copy link
Member

how about I disable the text specific tests on old GHCs?

Yes, that seems fair, especially since you add a new feature so it is less important to test it with old GHCs.

@andreasabel andreasabel added this to the 3.3.1.0 milestone Jun 17, 2023
Copy link
Member

@andreasabel andreasabel 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 this new feature!

I have some small change requests concerning the documentation.

Unfortunately, the docs are not built automatically from the PR even though I switched it on now at readthedocs.org. Probably it only works on new PRs...

doc/api.rst Outdated Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
doc/api.rst Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we merge this, try it out a bit, and then release.

Ok to squash?
If you want to preserve separate commits, please reorganize them so that there are no fixups and each commit has a clear purpose and commit message.
Otherwise, we squash them into one big PR commit.

@andreasabel andreasabel added the re: text Concerning using `Text` label Jun 20, 2023
@andreasabel andreasabel self-assigned this Jun 20, 2023
@alexbiehl
Copy link
Member Author

Cool! Squash is fine.

@andreasabel
Copy link
Member

I now tested this PR locally by porting one of my projects to the new posn-strict-text wrapper. Worked!

@andreasabel andreasabel merged commit a901757 into haskell:master Jun 20, 2023
45 checks passed
@alexbiehl alexbiehl deleted the alex/strict-text branch June 20, 2023 14:03
@andreasabel andreasabel removed this from the 3.3.1.0 milestone Jun 20, 2023
@andreasabel andreasabel added this to the 3.4.0.0 milestone Jun 20, 2023
@andreasabel
Copy link
Member

A new major version (3.4.0.0) looks more appropriate for a new feature, one can then have build-tool-depends: alex:alex >= 3.4 (rather than 3.3.1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
re: text Concerning using `Text`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants