bytestring
bytestring copied to clipboard
Add functionality for toConstr on Data instance
PR attempting to fix #513.
As discussed in the issue, I used the Data.Text implementation to copy from. I found the PR where the toConstr function was implemented, but it did not include any tests to help me write some for this PR. I just used the use case for the gshow function.
I looked at the implementation of gshow and saw that it was calling showConstr . toConstr. I added line to the tests and it seems to work fine.
Wrt to tests, the purpose of #513 was to use gshow on ByteString. Is it possible to add a test that gread . gshow == id?
@Bodigrim I've been looking into this and it seems like there is not a test for this on the Text repo. gread . gshow == id does not appear to be possible anyway when I test it with the Text package because [(a, String)] -> [(a, String)] does not have an Eq instance. I've looked into how gshow and gread works but I can't get gshow to output a legible string ((gshow . pack) "ABC" == "(pack \"ABC\")"). Currently gshow outputs a string based on the ascii number of each byte in the bytestring ((gshow . pack) "a" == "(pack ((:) (65) ([])))"). I've added a couple tests to show that at least gshow does not produce an error.
@Colton-Clemmer please add unit tests, e. g., that (gshow . pack) "a" == "(pack ((:) (65) ([])))" and similar.
@Bodigrim Test added
Miss clicked the close button...
@chshersh does this patch satisfy your expectations?
@Bodigrim it certainly does 👍🏻
Thanks @Colton-Clemmer for the implementation 🤗
@sjakobi I have hopefully resolved your code review items.
@cbclemmer I rebased your branch atop the latest master. Do you have any idea what's up with gread as noted by @sjakobi above?
@Bodigrim Honestly, I haven't come back to this because I realized I was trying to fix something I didn't understand and I probably shouldn't have made the pr in the first place. Someone can take this over if they want to but I've looked at the docs for gread and toConstr too much and still can't figure it out.
Thanks anyway for trying, @cbclemmer!