Skip to content

Indexed foldable-traversable instances #23

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 4 commits into from
Nov 21, 2017

Conversation

matthewleon
Copy link
Contributor

Addresses #21

@paf31
Copy link
Contributor

paf31 commented Nov 19, 2017

Thank you for the PR, but it's not clear to me that Semiring is always the right choice here. Certainly, to be compatible with the non-indexed instances, it needs to be the "first" index somehow, but I don't know if that means zero necessarily. Nothing says the indices even have to come in increasing order. That said, we don't really have any laws which tell us what the index should mean in these instances.

@matthewleon
Copy link
Contributor Author

Good points. Didn't think of this.

@matthewleon
Copy link
Contributor Author

Does this imply that #21 should actually be closed? It would appear that there is no sensible way to do what it's asking.

@paf31
Copy link
Contributor

paf31 commented Nov 20, 2017

I think so, thanks!

@safareli
Copy link

what about

mapWithIndex f (a :| fa) = f Nothing a :| mapWithIndex (Just >>> f) fa 

@paf31
Copy link
Contributor

paf31 commented Nov 20, 2017

Good idea, that seems much better.

@paf31 paf31 reopened this Nov 20, 2017
@matthewleon
Copy link
Contributor Author

@safareli awesome. I've modified these accordingly.

@safareli
Copy link

Maybe is algebraic "+ 1", and that's what we need to do with the index type.

Welcome and thanks for making this pr @matthewleon!

@paf31 paf31 merged commit 0b59029 into purescript:master Nov 21, 2017
@paf31
Copy link
Contributor

paf31 commented Nov 21, 2017

Thanks!

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.

3 participants