bytestring icon indicating copy to clipboard operation
bytestring copied to clipboard

Add functionality for toConstr on Data instance

Open cbclemmer opened this issue 3 years ago • 8 comments

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.

cbclemmer avatar Jun 17 '22 22:06 cbclemmer

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 avatar Jun 19 '22 10:06 Bodigrim

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

cbclemmer avatar Jul 03 '22 17:07 cbclemmer

@Colton-Clemmer please add unit tests, e. g., that (gshow . pack) "a" == "(pack ((:) (65) ([])))" and similar.

Bodigrim avatar Jul 03 '22 17:07 Bodigrim

@Bodigrim Test added

cbclemmer avatar Jul 04 '22 00:07 cbclemmer

Miss clicked the close button...

cbclemmer avatar Jul 04 '22 00:07 cbclemmer

@chshersh does this patch satisfy your expectations?

Bodigrim avatar Jul 04 '22 18:07 Bodigrim

@Bodigrim it certainly does 👍🏻

Thanks @Colton-Clemmer for the implementation 🤗

chshersh avatar Jul 05 '22 08:07 chshersh

@sjakobi I have hopefully resolved your code review items.

cbclemmer avatar Jul 05 '22 19:07 cbclemmer

@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 avatar Nov 14 '22 22:11 Bodigrim

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

cbclemmer avatar Nov 15 '22 03:11 cbclemmer

Thanks anyway for trying, @cbclemmer!

clyring avatar Nov 19 '22 04:11 clyring