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

Quote and escape in QCheck.Print.{char,string}, aligning it with QCheck2.Print #297

Merged
merged 9 commits into from
Dec 12, 2024

Conversation

jmid
Copy link
Collaborator

@jmid jmid commented Dec 6, 2024

As mentioned in #296

  • the QCheck.Observable.{char,string} function printers yield unhelpful output, despite using QCheck.Print.{char,string}
  • quickfixing it like Quote and escape Observable strings and chars #296, adds another ad-hoc copy, rather than fixing the root cause, namely QCheck.Observable.{char,string}
  • QCheck2.Print.{char,string} already quotes and escapes, creating a QCheck.Print / QCheck2.Print mismatch for chars and strings

This PR thus suggests the breaking change of fixing QCheck.Print.{char,string}.
This further opens up for cleaning up some of the ad-hoc printers in the arbitrary combinators (commit d1c1fee)

I'm hoping for fellow QCheck users and developers to chip in to the discussion and share which PR and QCheck2.Print.{char,string} they prefer... 🙏 🙂

@jmid
Copy link
Collaborator Author

jmid commented Dec 6, 2024

To assess if this change would break anything I've rolled an opam-repo PR ocaml/opam-repository#27038
The conclusion is clear: there were (the usual) failures but none caused by QCheck.Print.{char,string} printers changing to properly quote and escape.

Based on this experiment I therefore favor this one over #296. OK with you @c-cube?

@jmid jmid force-pushed the fix-qcheck-print-escaping branch from 1f67a37 to 3639d68 Compare December 7, 2024 12:40
@jmid
Copy link
Collaborator Author

jmid commented Dec 7, 2024

Rebased on main after merging #298

@jmid
Copy link
Collaborator Author

jmid commented Dec 7, 2024

Looking again at our workaround in multicoretests I was reminded that bytes has the same problem - in both QCheck.Print and in QCheck2.Print.

The latest commits thus fix Print.bytes too.

The expect test updates in d8f3b5f and f773f75 document why this helps the end-user output.

@jmid jmid force-pushed the fix-qcheck-print-escaping branch from 0640326 to 75f1061 Compare December 12, 2024 14:00
@jmid
Copy link
Collaborator Author

jmid commented Dec 12, 2024

Rebased on main after merging #302

@jmid
Copy link
Collaborator Author

jmid commented Dec 12, 2024

OK. Since no objections have surfaces I'll go ahead and merge this one.

@jmid jmid merged commit 6af3dc4 into c-cube:main Dec 12, 2024
13 checks passed
@jmid jmid deleted the fix-qcheck-print-escaping branch December 12, 2024 16:45
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.

1 participant