search

composewell / unicode-transforms

Fast Unicode normalization in Haskell
BSD 3-Clause "New" or "Revised" License
47 stars 16 forks source link

Support text-icu 0.8.0 for quickcheck tests (fixes #81) #82

Closed felixonmars closed 2 years ago

harendra-kumar commented 2 years ago

Looks good. A couple of questions:

  1. Have you tested if it builds with older versions of text-icu especially since the imported module has changed from Data.Text.ICU to Data.Text.ICU.Normalize for older versions.
  2. Do we need to upload a release after this merge to support the newer version of text-icu?
felixonmars commented 2 years ago
  1. The functions used are actually in Data.Text.ICU.Normalize since 13 years ago. The module was previously imported in Data.Text.ICU too though, so I made this more explicit in comparison to the new preferred Normalize2 module.
  2. That would be great :)
harendra-kumar commented 2 years ago

Thank you @felixonmars for the fix. I think we can merge it and upload a patch release.

adithyaov commented 2 years ago

The solver would never pick text-icu-0.8.0 because of text-icu >=0.6.2.1 && <0.8 in the benchmark bench. The cabal file needs to be updated accordingly.

Can you please confirm the following,

cabal build test:quickcheck --flag has-icu

works in the following cases,

  1. When text-icu == 0.8.0
  2. When text-icu == 0.7.1.0
felixonmars commented 2 years ago

Can you please confirm the following,

cabal build test:quickcheck --flag has-icu

works in the following cases,

1. When `text-icu == 0.8.0`

2. When `text-icu == 0.7.1.0`

Yes, I confirm that these combination works. I didn't really look into the benchmarks though.

adithyaov commented 2 years ago

Sorry for the delay in merging this. This completely slipped my mind and my task list. We still have to change the version bounds in the cabal file. I'll do that later.

Thanks, @felixonmars