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 elem combinator to API #274

Merged
merged 1 commit into from
Mar 20, 2021
Merged

Add elem combinator to API #274

merged 1 commit into from
Mar 20, 2021

Conversation

emilypi
Copy link
Member

@emilypi emilypi commented Jan 7, 2020

This PR adds the elem combinator to the API.

Data/Text.hs Outdated Show resolved Hide resolved
@hvr
Copy link
Member

hvr commented Apr 12, 2020

So I'm slightly on the fence about this; for one, elem is trivially definable in terms of any

elem c = any (==c)

and I'm not sure if this meets the https://wiki.haskell.org/Fairbairn_threshold standard.

Fwiw, there's appears to be lack of evidence that this is needed insofar as nobody seems to have missed having this trivial definition in Data.Text for over a decade...

However, there's precedent in e.g. Data.ByteString.elem and Data.Vector.elem which do provide this standard verb; so for the sake of API symmetry/uniformity among vector/list-ish data-structures, it'd make sense to add this (redundant) verb, as there might be some level of expectation of it existing.

Also, in principle, elem could be implemented more efficiently in some cases where fusion doesn't matter via a memmem(3) substring search on the utf16 encoding.

@emilypi
Copy link
Member Author

emilypi commented Apr 13, 2020

So I'm slightly on the fence about this; for one, elem is trivially definable in terms of any

My reasoning is precisely that this is a part of the API that's defined for every other string-like object or list-like container. It passes the Fairbarn test on account of it's expected to be there, eminently useful (perhaps less so for text), and it homogenizes the APIs across libraries. You may object to it on the bounds that it's a small addition, but it was surprising that it was not already.

@emilypi
Copy link
Member Author

emilypi commented Dec 9, 2020

@hvr updated and it should be ready to go

@emilypi
Copy link
Member Author

emilypi commented Dec 9, 2020

@phadej too, if you're more active

@emilypi emilypi force-pushed the topos/elem branch 2 times, most recently from ca402e4 to 378bd74 Compare January 30, 2021 19:09
author Emily Pillmore <emily@kadena.io> 1578367360 -0500
committer Emily Pillmore <emilypi@cohomolo.gy> 1612033171 -0500

Add elem combinator to api
@emilypi
Copy link
Member Author

emilypi commented Jan 30, 2021

@hvr rebased to master so we should be able to just merge with no problems

@Lysxia
Copy link
Contributor

Lysxia commented Mar 15, 2021

This looks good to me (conclusion: we want this for uniformity with other collection-like APIs). Ping @haskell/text for another approval to merge this.

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.

Adding a test would be nice.

@Lysxia Lysxia merged commit 0998ad0 into haskell:master Mar 20, 2021
@Lysxia Lysxia mentioned this pull request Mar 20, 2021
2 tasks
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.

None yet

4 participants