Skip to content

Purescript 0.9.1 update #30

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
Jun 10, 2016
Merged

Purescript 0.9.1 update #30

merged 3 commits into from
Jun 10, 2016

Conversation

kika
Copy link
Contributor

@kika kika commented Jun 10, 2016

Please consider this PR for update to 0.9.1 compiler.
Fixes #29

@paulyoung
Copy link

I was about to do this myself.

},
"devDependencies": {
"purescript-console": "~0.1.1"
"purescript-console": "~1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use ^ rather than ~ for all dependencies please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. I left the range specs as they were there previously.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's a mixture of: bower used to default to ~, and it didn't matter as we were on 0.x.y anyway. As of the v1 releases using ^ means we can make additive changes to libraries without having to re-release things further down the chain.

@garyb
Copy link
Member

garyb commented Jun 10, 2016

@paf31 / @hdgarrood do you think we should be using JSDate or DateTime in this library? I'm torn...

@@ -31,24 +31,28 @@ module Node.FS.Async
, fdClose
) where

import Prelude
import Control.Monad.Eff
import Prelude (Unit, bind, show, flip, ($), (<<<), (/), (<$>))
Copy link
Member

Choose a reason for hiding this comment

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

Can you use an open import for Prelude please?

@hdgarrood
Copy link
Member

Since it's easy to convert between the two, and given that quite a few people use this library directly, I'm tempted to say DateTime. We can always introduce new higher or lower level APIs later if it turns out they would be useful.

@garyb
Copy link
Member

garyb commented Jun 10, 2016

Ok, cool. That was my thinking on the pro-using-DateTime too.

1. Caret ranges in bower.json instead of tilde
   #30 (comment)
2. Open import for Prelude
   #30 (comment)
@kika
Copy link
Contributor Author

kika commented Jun 10, 2016

Import for Prelude opened (is this the current convention?), tilde ranges replaced with caret ones.

Not sure if there's work required on the JSData/DateTime: the low level stuff is in JSDate and all computations are in DateTime and friends. Seems very nice and logical to me.

@hdgarrood
Copy link
Member

It is - the compiler lets you have one open import without warnings now, and so having Prelude as an open import means that you don't have to be updating the import list as often, and also reduces the risk of merge conflicts.

import Prelude
import Data.Maybe
import Data.Either
import Prelude (Unit, bind, (==))
Copy link
Member

Choose a reason for hiding this comment

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

If we make this open, can we get rid of the imports of Control.Apply and Control.Bind too?

1. Getting rid of `import Control.Apply` and `.Bind`
   #30 (comment)
2. Prelude made open in test/ (forgotten change)
@kika
Copy link
Contributor Author

kika commented Jun 10, 2016

Done. Didn't know that compiler allows one open import. Very convenient.

@hdgarrood while I have your attention: in whose court is the purescript-node-http ball? I'm using my own copy (and own change which I did just to discover that @paf31 already submitted the PR), but all the requirements for @paf31's PR have been met as it seems.
And there's also purescript-parsing in a strange state: all changes made, but not version tagged.

@garyb
Copy link
Member

garyb commented Jun 10, 2016

purescript-parsing isn't quite done, there's a PR changing it to use List which is much more efficient. If I get a chance this afternoon I'll cherry-pick the commit and open a new PR for that change.

@kika
Copy link
Contributor Author

kika commented Jun 10, 2016

@garyb I thought this PR make the build fail (though I believe it's just bad timing, infamous -ffi option) and thus was left aside.

@hdgarrood
Copy link
Member

Looks great. Thanks very much!

@hdgarrood hdgarrood merged commit 9b11c60 into purescript-node:master Jun 10, 2016
@hdgarrood
Copy link
Member

Re node-http, I think that PR looks fine, and the libraries we were waiting for have been updated, so I think now it just needs someone to update bower.json to point to real versions and tag it.

@paf31
Copy link
Contributor

paf31 commented Jun 10, 2016

I can look at this later today unless someone beats me to it.

@paulyoung
Copy link

Will these changes make it into a release any time soon?

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.

5 participants