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.

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

Map #1813

Merged
merged 15 commits into from
Jul 7, 2016
Merged

Map #1813

merged 15 commits into from
Jul 7, 2016

Conversation

kylebrandt
Copy link
Member

still needs docs

@CheRuisiBesares
Copy link
Contributor

So with this would all math functions going forward operate on numberSets like abs? I have to read the code a little more but looking at your test that seems to be the direction. You like this direction better than making another type or something that works more like an interface in go. Im thinking something like models.Iterable where functions could take something like abs(Iterable) Iterable where math functions can never change the type ie do what reductions are doing. Im just wondering I think you tried that in another branch. I was wondering what your experience was there.

@kylebrandt
Copy link
Member Author

@CheRuisiBesares This does solve that issue as a side effect but has more power. For instance, you can abs(v) - v**2 etc...

This doesn't have to be independent of making those functions accept both, we could do that. When I tried I had issue getting the branch to work. I think that maybe It requires that the parser does additional lookaheads to know when the return type is going to be. I was having trouble pulling that off. Although maybe after going through this and understanding this part of the code more it would be easier.

Since we could still change math functions to take (and return, that is the harder part) multiple types down the road independently of map, what this effectively does is lower the priority on caring about that for now. When someone has some alerts to show that use a lot of these functions, and map is making it ugly as hell, then I think it would be better to figure out how to implement it at that point since we would have more user stories at that point.

@CheRuisiBesares
Copy link
Contributor

Good stuff! Makes sense to me. Excited to get my TI-89 on v**2 + v + 1

@captncraig
Copy link
Contributor

Looks ok to me.

@kylebrandt kylebrandt merged commit d602113 into master Jul 7, 2016
@kylebrandt kylebrandt deleted the map branch July 26, 2016 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants