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.

Sign up for GitHub

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

Jump to bottom

Implement all the dict extra functions #7

Merged
merged 6 commits into from
Apr 24, 2017
Merged

Implement all the dict extra functions #7

merged 6 commits into from
Apr 24, 2017

Conversation

dawehner
Copy link
Contributor

This PR solves #3 aka. it implements all the methods from dict extra:

        , groupBy
        , fromListBy
        , removeWhen
        , removeMany
        , keepOnly
        , mapKeys

The test coverage for now is mostly unit test coverage as my knowledge of fuzz tests is still limited.

@amitaibu
Copy link
Member

Wow, nice! @rgrempel is probably going to be happy :)

@dawehner
Copy link
Contributor Author

Well, the missing quickchecks are a bit sad

@amitaibu
Copy link
Member

Are you already using it in a project, or just geeking out?

@dawehner
Copy link
Contributor Author

I'm totally just geeking out, but I see it to be useful on my new project: a https://lobste.rs/ clone. I guess its also really related with your items and item managers work.

@amitaibu
Copy link
Member

I guess its also really related with your items and item managers work.

The items manager is mostly about fetching and unwraping items from WebData, so when it hands it down to sub-components they get a non-wrapped Item (thus an easier one to interact with)

@dawehner
Copy link
Contributor Author

dawehner commented Mar 21, 2017 •
edited
Loading

@amitaibu The problem I tried to solve was a pager + updating the list of comments one wants to render.

@amitaibu
Copy link
Member

Then maybe you'd like to give this a stab?

@dawehner
Copy link
Contributor Author

Interesting, thank you @amitaibu !

rgrempel
rgrempel reviewed Mar 21, 2017
View reviewed changes
Copy link
Collaborator

@rgrempel rgrempel left a comment

Choose a reason for hiding this comment

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

Thanks again!

src/DictList.elm Outdated
-}

import Dict exposing (Dict)
import DictList.Compat exposing (customDecoder, decodeAndThen, first, maybeAndThen, second)
import Json.Decode exposing (Decoder, keyValuePairs, value, decodeValue)
import List.Extra
import Set
Copy link
Collaborator

Choose a reason for hiding this comment

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

May as well add exposing (Set), I think.

src/DictList.elm Outdated
groupBy .id [mary, jack, jill] == DictList.fromList [(1, [mary, jill]), (2, [jack])]
-}
groupBy : (comparable1 -> a -> comparable2) -> DictList comparable1 a -> DictList comparable2 (List a)
groupBy keyfn list =
Copy link
Collaborator

@rgrempel rgrempel Mar 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I think what we'd be looking for here is something like this instead:

groupBy : (a -> comparable) -> List a -> DictList comparable (List a)

That is, I think the input should still be a List, not another DictList. So, the signature would be the same except that we produce a DictList instead of a Dict. And, the additional behaviour would be that the resulting DictList preserves the order of the incoming list (which Dict.Extra.groupBy would not do).

Part of the logic is that we're in Dict.Extra, so it is (generally speaking) the Dict parameters that we want to be DictList -- the List things here really are meant to be good-old-lists (though there may be some exceptions).

Or, to put it another way, the point of groupBy is to add keys to a list of values, so we need not require that we already have keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with you. Its not only that this is more parallel to DictExtra, but more important, this makes the function more useful for many usecases.

src/DictList.elm Outdated
jill = {id=1, name="Jill"}
fromListBy .id [mary, jack, jill] == DictList.fromList [(1, jack), (2, jill)]
-}
fromListBy : (comparable1 -> a -> comparable2) -> DictList comparable1 a -> DictList comparable2 a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above -- I think this makes more sense as really being a way to construct a DictList from a List, not from another DictList. As written here, it's more of a variation on a map ... I suppose we could have another function and call it something like mapKeys if it's useful. Ah, except that as I read on we already have a mapKeys that just considers the key. I suppose we could have a mapKeysAndValues, in theory -- I haven't really thought it through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, yeah I guess such a generic transformation function could be helpful, but indeed I think this doesn't have to be addressed as part of this PR.

tests/src/DictExtraTests.elm
@@ -0,0 +1,98 @@
module DictExtraTests exposing (tests)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks pretty good. I'd also suggest having a separate test file that copies https://github.com/elm-community/dict-extra/blob/1.3.2/tests/Main.elm and makes whatever the smallest changes are needed to match our context here. Basically, to prove that we pass Dict.Extra's own tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done that.

@dawehner
Copy link
Contributor Author

I pushed some changes which should address all the points from you @rgrempel

@rgrempel
Copy link
Collaborator

Are you sure you pushed the changes? I'm not seeing them ...

@dawehner
Copy link
Contributor Author

dawehner commented Mar 29, 2017 via email

@rgrempel
Copy link
Collaborator

rgrempel commented Apr 3, 2017

Thanks, this looks pretty good at first glance. I'll merge it as soon as I get a chance to review more carefully.

@rgrempel
Copy link
Collaborator

Sorry this took 3 weeks for me to get back to -- looks good!

@rgrempel rgrempel merged commit 076f7ac into Gizra:master Apr 24, 2017
@rgrempel rgrempel mentioned this pull request Apr 24, 2017
Closed
@rgrempel
Copy link
Collaborator

Published as version 1.1 ... thanks again, @dawehner.

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

@rgrempel rgrempel rgrempel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dawehner @amitaibu @rgrempel