Skip to content

Map filter #11239

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

Merged
merged 20 commits into from
Sep 12, 2021
Merged

Map filter #11239

merged 20 commits into from
Sep 12, 2021

Conversation

Qqwy
Copy link
Contributor

@Qqwy Qqwy commented Sep 11, 2021

PR is based on this proposal.

  • Implementation of Map.filter/2 and Map.reject/2.
  • Implementation of the analogous Keyword.filter/2 and Keyword.reject/2
  • Map.map/2
  • Keyword.map/2

Questions:

  • :maps.filter/2 supports a map iterator as first parameter. As we're not currently exposing map iterators through the Map module at all, my assumption is that this support is not required for Map.filter/2 etc., simplifying the code a little. Is this correct?
  • :maps.filter/2's source code has some error handling/decorating. I'm not 100% sure how to convert this to Elixir code. Is this already handled by Elixir's own FunctionClause-handling where "function clauses that were tried" are printed? We might be able to remove this line from the implementation, maybe?

@josevalim
Copy link
Member

About your questions, yes, we don’t need to support iterators and function clause errors are good enough!

@Qqwy
Copy link
Contributor Author

Qqwy commented Sep 11, 2021

I believe the implementation is finished.

Are there still places where e.g. the formatting of the documentation needs improvement?

Copy link
Contributor

@christhekeele christhekeele left a comment

Choose a reason for hiding this comment

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

I have just a few documentation formatting thoughts if interested, this looks great

Copy link
Contributor

@v0idpwn v0idpwn left a comment

Choose a reason for hiding this comment

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

I think these changes could improve the documentation a bit.

While the moduledoc for Keyword specifies that functions don't guarantee ordering, I think maybe it could be good to point that in the new functions as it may be counter intuitive based on the name.

Qqwy and others added 9 commits September 11, 2021 20:33
Superfluous newline

Co-authored-by: felipe stival <14948182+v0idpwn@users.noreply.github.com>
Co-authored-by: felipe stival <14948182+v0idpwn@users.noreply.github.com>
Co-authored-by: felipe stival <14948182+v0idpwn@users.noreply.github.com>
Co-authored-by: felipe stival <14948182+v0idpwn@users.noreply.github.com>
Co-authored-by: felipe stival <14948182+v0idpwn@users.noreply.github.com>
@josevalim josevalim merged commit ee4852e into elixir-lang:master Sep 12, 2021
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@s3cur3
Copy link
Contributor

s3cur3 commented Nov 3, 2021

Should we document what happens when Map.map/2 produces collisions in the keys? Given that the current implementation uses :maps.from_list/2, we could just use the same verbiage as Map.new/2:

Duplicated keys are removed; the latest one prevails.

(I suppose we might conceivably not want to make any guarantees in this area and just note that collisions in keys will cause all but one instance of the key to be dropped, hinting that maybe one shouldn’t don’t do that.)

@josevalim
Copy link
Member

@s3cur3 yes, a PR is welcome!

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.

5 participants