Skip to content

Conversation

@SteffenDE
Copy link
Contributor

Discussed on https://groups.google.com/g/elixir-lang-core/c/JtWvn9aghgQ

The code addition itself is quite small.

Copy link
Contributor

@sabiwara sabiwara left a comment

Choose a reason for hiding this comment

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

LGTM 💜

@whatyouhide whatyouhide changed the title add String.count/2 Add String.count/2 Apr 24, 2025
SteffenDE added a commit to SteffenDE/stream_data that referenced this pull request Apr 24, 2025
@SteffenDE SteffenDE changed the title Add String.count/2 Add String.count_matches/2 Apr 24, 2025
@wojtekmach
Copy link
Member

This reminds me a little bit of Enum.frequencies and I’m curious if String.frequency would make for the right association. I think String.frequencies would be an easier sell but it’s obviously different functionality. I think the current name is good, especially using word count, but thought I’d mention this for completes.

@josevalim
Copy link
Member

Apologies for the back and forth. After some discussion we decided to stick with String.count/2. The reason is that all of the APIs that receive the exact same type of argument, such as String.split and String.replace, do not have _matches or _occurrences in the name. So if we add String.count_matches, people may wonder if it behaves differently than split and replace, while the arguments and the properties are the same.

@josevalim
Copy link
Member

I'd also add that, while String.count may be unclear, String.count/2 removes the ambiguity because the additional argument tells you that something more is going on, similar to Enum.count/2.

@josevalim josevalim changed the title Add String.count_matches/2 Add String.count/2 Apr 25, 2025
@josevalim josevalim merged commit 0fb90f1 into elixir-lang:main Apr 25, 2025
11 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

8 participants