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

Add dropWhileEnd, takeWhileEnd, strip #121

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Add dropWhileEnd, takeWhileEnd, strip #121

merged 1 commit into from
Jul 1, 2020

Conversation

nmattia
Copy link
Contributor

@nmattia nmattia commented Apr 9, 2017

A few notes:

  • I couldn't build the benchmarks, so I haven't added benchmarks for the new functions. However their implementation is very similar to that of other functions so it shouldn't make too big a difference (though I don't know if I should inline strip).
  • I don't know how closely Data.ByteString.Char8 is trying to follow the functions on String: for instance, there is no strip function on String. Let me know whether exporting the strip and dropSpace functions is the right thing to do or not (though my motivation for adding dropWhileEnd and takeWhileEnd is to have strip).
  • I haven't implemented dropWhileEnd and takeWhileEnd on lazy bytestrings, because (1) I don't know how much sense it makes and (2) I wouldn't know how to do that efficiently, for some definition of efficiently.

@nmattia
Copy link
Contributor Author

nmattia commented Apr 9, 2017

Note: this should address #19.

@bgamari
Copy link
Contributor

bgamari commented Feb 18, 2018

@dcoutts, it seems like this would be rather nice to have.

@typesanitizer
Copy link

I haven't implemented dropWhileEnd and takeWhileEnd on lazy bytestrings, because (1) I don't know how much sense it makes and (2) I wouldn't know how to do that efficiently, for some definition of efficiently.

Lazy Text has both dropWhileEnd and takeWhileEnd. Since the chunked representations of lazy Text and lazy ByteString are very similar, I think it makes sense for bytestring to provide the corresponding functions. Similarly, lazy Text also has a strip function, so maybe it should be added too.


The strip implementation in the PR uses dropWhile isSpace which could be replaced by dropSpace (there is a rewrite rule for this so it shouldn't matter in practice). There is also a dropSpaceEnd function commented out (I'm not sure why it is commented out though) which could be used here.

@hvr
Copy link
Member

hvr commented Feb 23, 2019

A related PR is #155 adding findIndexEnd

@nmattia
Copy link
Contributor Author

nmattia commented Mar 30, 2019

What should we do here? This has been open for 2 years and I'd like to delete my bytestring fork

@nmattia
Copy link
Contributor Author

nmattia commented Jun 1, 2019

Gentle ping

@nmattia
Copy link
Contributor Author

nmattia commented Jul 1, 2019

@dcoutts ping!

@relrod
Copy link

relrod commented Sep 10, 2019

+1 for these; anything I (as a community member) could do to help move this along?
/cc @dcoutts

@nmattia
Copy link
Contributor Author

nmattia commented Dec 19, 2019

@hvr @dcoutts ping!

Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Looks good to me. We can raise a separate issue to discuss an implementation of {take,drop}WhileEnd on lazy ByteStrings, but I do not think this should be a blocker for merging. CC @cartazio

@Bodigrim
Copy link
Contributor

@nmattia could you please update changelog?

@nmattia
Copy link
Contributor Author

nmattia commented Jun 25, 2020

@Bodigrim done, apologies about the delay!

@sjakobi sjakobi linked an issue Jun 25, 2020 that may be closed by this pull request
@nmattia
Copy link
Contributor Author

nmattia commented Jun 29, 2020

Fixed conflicts, shall we merge?

@sjakobi
Copy link
Member

sjakobi commented Jun 29, 2020

@nmattia We're currently preparing a bug fix release for GHC 8.8.4 and 8.10.2: #235

I'd like to merge this very soon after that release.

@hsyl20, @bgamari Would you like to review?

Copy link
Contributor

@hsyl20 hsyl20 left a comment

Choose a reason for hiding this comment

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

LGTM and tests pass locally too. Thanks @nmattia!

@nmattia
Copy link
Contributor Author

nmattia commented Jul 1, 2020

Updated changelog! @sjakobi anything else I can do?

@Bodigrim
Copy link
Contributor

Bodigrim commented Jul 1, 2020

@nmattia I'll merge this, once we get 0.10.10.1 out (hopefully today).

@Bodigrim Bodigrim merged commit 3171311 into haskell:master Jul 1, 2020
@Bodigrim
Copy link
Contributor

Bodigrim commented Jul 1, 2020

Merged, thanks for your patience!

@nmattia nmattia deleted the strip branch July 6, 2020 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dropWhileEnd
8 participants