Skip to content

Topic/additional functions #20

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 3 commits into from
Nov 30, 2014

Conversation

ethul
Copy link
Contributor

@ethul ethul commented Nov 29, 2014

I salvaged a few functions from purescript-angular before removing the ST module there. I had been debating sending this PR, but perhaps you might find these useful (as per ethul/purescript-angular#5).

I also added getElems and getAssocs. I was on the fence about these, so if you don't think they are needed, I can definitely remove them. Additionally, I've changed the return type of pushSTArray to Number from Unit. I am not sure if this was the right thing to do, but I could go either way.

@paf31
Copy link
Contributor

paf31 commented Nov 29, 2014

Thanks! This looks great.

I was going to say that I thought getElems was unsafe, but now I see that you're copying the array, which is good. I wonder about the name though - in Haskell, I believe it would be called freeze, which I quite like.

Also, maybe getAssocs could be renamed to toAssocArray? Just a thought, totally up for discussion...

@ethul
Copy link
Contributor Author

ethul commented Nov 30, 2014

Welcome!

I wasn't actually sure if it should be getElems or freeze. I am open to either. Both exist in Haskell. If you have a preference, I can make the name change.

For getAssocs, I'd be happy to rename it to toAssocArray, but I was just keeping with the function name in Haskell. Should we depart from the Haskell name?

@ethul
Copy link
Contributor Author

ethul commented Nov 30, 2014

Actually, it looks like getElems and getAssocs are on Data.Array.MArray. I don't think I am familiar enough with these mutable array modules in Haskell yet to make a good naming choice here. What do you think? I am happy to go with your suggestions here.

@paf31
Copy link
Contributor

paf31 commented Nov 30, 2014

Oops, I guess I don't know MArray as well as I thought.

The only reason I would prefer freeze is that it would be consistent with the new functions in Data.Map.ST:

https://github.com/purescript/purescript-maps/blob/master/src/Data/StrMap.purs#L81

@ethul
Copy link
Contributor Author

ethul commented Nov 30, 2014

Sounds good. I've updated the names and added thaw.

paf31 added a commit that referenced this pull request Nov 30, 2014
@paf31 paf31 merged commit 00ff8d2 into purescript:master Nov 30, 2014
@paf31
Copy link
Contributor

paf31 commented Nov 30, 2014

👍 Thanks!

This makes me wonder if we should also

  • Rename the existing functions: pushSTArray is unnecessarily verbose IMO - push would be enough, given qualified imports.
  • Add a copy function: copy = freeze >=> thaw

@ethul
Copy link
Contributor Author

ethul commented Nov 30, 2014

Thanks!

I think the function renaming is a great idea. I am in favour of the shorter names and using the module name for context.

Good call on copy too!

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.

2 participants