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.

Sign up for GitHub

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

Jump to bottom

add unzip and elemIndexEnd to Data.ByteString.Lazy.Char8 #294

Closed
wants to merge 6 commits into from
Closed

add unzip and elemIndexEnd to Data.ByteString.Lazy.Char8 #294

wants to merge 6 commits into from

Conversation

emmanueldenloye
Copy link

This addresses #104

I would greatly appreciate all criticism!

@emmanueldenloye emmanueldenloye changed the title add unzip and elemIndexEnd to Data.ByteStrintg.Lazy.Char8 add unzip and elemIndexEnd to Data.ByteString.Lazy.Char8 Oct 2, 2020
@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 2, 2020

@emmanueldenloye Please ensure that Travis build is all green, even for older GHCs. They do not expose <$> from Prelude: https://travis-ci.org/github/haskell/bytestring/jobs/732140582#L806

@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 3, 2020

Cool! Please add tests as well.
https://github.com/haskell/bytestring/blob/master/tests/Properties.hs

@emmanueldenloye
Copy link
Author

I found it strange that there weren't test for find, findIndex, and findIndices for the respective functions in Data.ByteString.Lazy.Char8. Is that an acceptable omission?

@Bodigrim
Copy link
Contributor

Unfortunately, I suspect that there are many such test omissions. In the current state of the test suite it is very difficult to ensure that all four flavours of ByteString are uniformly covered with tests. This could be worth fixing in a separate PR.

Bodigrim
Bodigrim reviewed Oct 11, 2020
View reviewed changes
Copy link
Contributor

@Bodigrim Bodigrim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Cool, almost there!
Please resolve merge conflicts.

tests/Properties.hs Outdated
Just i -> Just (fromIntegral (length xs) -1 -i))

prop_elemIndexEnd2LC c xs = (LC.elemIndexEnd c (LC.pack xs)) ==
((-) (fromIntegral (length xs) - 1) `fmap` LC.elemIndex c (LC.pack $ reverse xs))
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between prop_elemIndexEnd1LC and prop_elemIndexEnd2LC? They look equivalent to me.

Copy link
Author

Choose a reason for hiding this comment

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

I saw a duplicate test elsewhere (look for prop_elemIndexEnd1LL and prop_elemIndexEnd2LL. I can remove the duplicates in both places if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Yes, please remove other duplicates as well.

Data/ByteString/Lazy/Char8.hs
-- satisfying the predicate.
findIndexEnd :: (Char -> Bool) -> ByteString -> Maybe Int64
findIndexEnd f = L.findIndexEnd (f . w2c)
{-# INLINE findIndexEnd #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I did not realise that findIndexEnd is missing. Please add it to Data.ByteString.Char8 as well, and write a test.

@Bodigrim Bodigrim linked an issue Oct 11, 2020 that may be closed by this pull request
Add elemIndexEnd, findIndexEnd and unzip to Data.ByteString.Lazy.Char8 #104
Closed
@Bodigrim
Copy link
Contributor

@emmanueldenloye do you need any help with this? I'm sorry, if my review suggestions were unclear, I am happy to elaborate if needed.

@emmanueldenloye
Copy link
Author

I'm sorry. I just got really busy at work. I'm going to attend to this PR again this weekend. Sorry for the holdup.

@emmanueldenloye
(HELP) prop_findIndexEndLC is not passing
8a445e9 
I've checked the expected values and they seem correct. However the
test is still failing for some reason that I cannot decipher. I
figured I would publish this commit just to move this PR forward.
@emmanueldenloye
Merge branch 'master' of https://github.com/haskell/bytestring into e…
b30b7e8 
…mmanuel-add-elemindexend-and-unzip
@emmanueldenloye
Copy link
Author

@Bodigrim My commit 8a445e9 (shortened) has a failing test and I'm not sure why. There are some more comments within the commit message.

@Bodigrim
Copy link
Contributor

https://travis-ci.org/github/haskell/bytestring/jobs/738655659#L659

Properties.hs:1388:71: error:
    Not in scope: ‘LC.unzip’
    Perhaps you meant one of these:
      ‘C.unzip’ (imported from Data.ByteString.Char8),
      ‘L.unzip’ (imported from Data.ByteString.Lazy),
      ‘LC.zip’ (imported from Data.ByteString.Lazy.Char8)
    Module ‘Data.ByteString.Lazy.Char8’ does not export ‘unzip’.
     |
1388 | prop_unzipLC x = let (xs,ys) = unzip x in (LC.pack xs, LC.pack ys) == LC.unzip x
     |                                                                       ^^^^^^^^

Seems like a bad merge: your commit b30b7e8 has accidentally removed unzip. Could you please restore it in a new commit?

@Bodigrim
Copy link
Contributor

@emmanueldenloye is it still in flight? Do you intend to continue with this PR?

@emmanueldenloye
Copy link
Author

Yes, I do. I plan to resume today in fact. Thank you for asking.

@Bodigrim Bodigrim mentioned this pull request Jan 9, 2021
Merged
@Bodigrim
Copy link
Contributor

Closed in favor of #342. Thanks for your efforts nevertheless!

@Bodigrim Bodigrim closed this Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bodigrim Bodigrim Bodigrim left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add elemIndexEnd, findIndexEnd and unzip to Data.ByteString.Lazy.Char8
2 participants
@emmanueldenloye @Bodigrim