Skip to content

Add elem combinator to API - #274

Merged
Lysxia merged 1 commit into
haskell:masterfrom
emilypi:topos/elem
Mar 20, 2021
Merged

Add elem combinator to API#274
Lysxia merged 1 commit into
haskell:masterfrom
emilypi:topos/elem

Conversation

@emilypi

@emilypi emilypi commented Jan 7, 2020

Copy link
Copy Markdown
Member

This PR adds the elem combinator to the API.

Comment thread Data/Text.hs Outdated
@hvr

hvr commented Apr 12, 2020

Copy link
Copy Markdown
Member

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

emilypi commented Apr 13, 2020

Copy link
Copy Markdown
Member Author

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

emilypi commented Dec 9, 2020

Copy link
Copy Markdown
Member Author

@hvr updated and it should be ready to go

@emilypi

emilypi commented Dec 9, 2020

Copy link
Copy Markdown
Member Author

@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

emilypi commented Jan 30, 2021

Copy link
Copy Markdown
Member Author

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

@Lysxia

Lysxia commented Mar 15, 2021

Copy link
Copy Markdown
Contributor

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.

@Bodigrim Bodigrim left a comment

Copy link
Copy Markdown
Contributor

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.

4 participants