add Indexl and indexr by safareli · Pull Request #83 · purescript/purescript-foldable-traversable · GitHub
Skip to content

add Indexl and indexr#83

Merged
garyb merged 3 commits into
purescript:masterfrom
safareli:index
Jan 9, 2018
Merged

add Indexl and indexr#83
garyb merged 3 commits into
purescript:masterfrom
safareli:index

Conversation

@safareli

Copy link
Copy Markdown
Contributor

fixes #82

@safareli

Copy link
Copy Markdown
Contributor Author

Comment thread src/Data/Foldable.purs
indexr :: forall a f. Foldable f => Int -> f a -> Maybe a
indexr idx = _.elem <<< foldr go { elem: Nothing, pos: 0 }
where
go elem cursor =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could create non-exported indexGo function and use it in in both places (in one I would need to flip it)

@safareli

Copy link
Copy Markdown
Contributor Author

last commit changed output from:

var go = function (v) {
    return function (v1) {
        if (v.pos === idx) {
            return {
                pos: v.pos + 1 | 0,
                elem: new Data_Maybe.Just(v1)
            };
        };
        var $119 = {};
        for (var $120 in v) {
            if ({}.hasOwnProperty.call(v, $120)) {
                $119[$120] = v[$120];
            };
        };
        $119.pos = v.pos + 1 | 0;
        return $119;
    };
};

to

var go = function (cursor) {
    return function (elem1) {
        if (cursor.elem instanceof Data_Maybe.Just) {
            return cursor;
        };
        var $115 = cursor.pos === idx;
        if ($115) {
            return {
                elem: new Data_Maybe.Just(elem1),
                pos: cursor.pos
            };
        };
        return {
            pos: cursor.pos + 1 | 0,
            elem: cursor.elem
        };
    };
};

@safareli

Copy link
Copy Markdown
Contributor Author

@paf31 it's ready

@garyb

garyb commented Nov 28, 2017

Copy link
Copy Markdown
Member

Wouldn't this sort of thing be better suited to FoldableWithIndex, etc? I'm not sure adding indexing to a "normal" Foldable is a good idea necessarily, since I don't think there's any implied order, so ideas like getting the head is a bit strange.

@paf31

paf31 commented Nov 28, 2017

Copy link
Copy Markdown
Contributor

I do think it makes sense to add this. A Foldable instance for a container just iterates (some or all of) its elements in some order. You can definitely observe the order by using toList, for example.

@safareli

Copy link
Copy Markdown
Contributor Author
takel idx == toList >>> index idx 
taker idx == toList >>> \l -> index (length l - idx )

@matthewleon

Copy link
Copy Markdown
Contributor

I do think it makes sense to add this. A Foldable instance for a container just iterates (some or all of) its elements in some order. You can definitely observe the order by using toList, for example.

Maybe with a warning that people using this on, say, a Map structure should have no guarantee that the order can't change in a point release?

@paf31

paf31 commented Nov 30, 2017

Copy link
Copy Markdown
Contributor

I'm not sure. It would be quite annoying, I think, for the behavior of say, an Ord instance to change in a minor release. I would expect this to be the same.

@safareli

Copy link
Copy Markdown
Contributor Author

I think changing Ord behavior is braking change not minor. so if someone makes braking change in minor release, it's fault of the lib not of this function. Someone would still have same issue if toList is used for example.

@paf31

paf31 commented Nov 30, 2017

Copy link
Copy Markdown
Contributor

I'm not saying that it's the fault of the function, I'm saying the order of a fold should be considered part of the observable behavior of a Foldable instance for the purposes of semver.

@safareli

safareli commented Jan 8, 2018

Copy link
Copy Markdown
Contributor Author

@garyb garyb merged commit 36c0431 into purescript:master Jan 9, 2018
@safareli safareli deleted the index branch January 9, 2018 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add index :: forall a f. Foldable => f a -> Int -> Maybe a

4 participants