Skip to content
This repository was archived by the owner on Jun 10, 2021. It is now read-only.

upgrade for purescript 0.10 #25

Merged
merged 2 commits into from
Jan 10, 2017

Conversation

zhangchiqing
Copy link
Contributor

@zhangchiqing zhangchiqing commented Jan 10, 2017

Could you review my PR @SimonRichardson ?

@@ -10,24 +10,14 @@ import Control.Monad.Eff.Exception

import Data.Argonaut (printJson)
import Data.Argonaut.Core (Json(..))
import Data.Argonaut.Decode (DecodeJson, decodeJson)
-- import Data.Argonaut.Decode (DecodeJson, decodeJson)
Copy link
Owner

Choose a reason for hiding this comment

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

Delete this line pls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these examples are broken and I've basically converted all the examples into test cases, do you think we still need them?

We could just add a link to Readme.MD, saying "please check out the tests for more examples?"

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I think that's a good idea!

= "ok" := boolToJsNumber w.success
~> "n" := w.total
~> "nInserted" := fromMaybe 0.0 w.inserted
~> "nModified" := fromMaybe 0.0 w.modified
Copy link
Owner

Choose a reason for hiding this comment

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

👍 Awesome, I noticed this was broken as well.

Copy link
Contributor Author

@zhangchiqing zhangchiqing Jan 10, 2017

Choose a reason for hiding this comment

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

Thanks :)

BTW, I have a question: I used 0.0 here instead of 0 because WriteResult takes Maybe Number, and 0 is a Int not a Number. But I don't understand why I can't create a Number by (0 :: Number), it doesn't typecheck. That's why I used 0.0. The same trick I used here

Copy link
Owner

Choose a reason for hiding this comment

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

I think that's purescript works.

liftEff $ log "should update"
col <- collection "events" database
res <- updateOne ["name" B.:= "Wow"] ["name" B.:= "lol"] defaultUpdateOptions col
liftEff $ assert $ (obj [ "ok" A.:= 1, "n" A.:= 1, "nModified" A.:= 1, "nInserted" A.:= 0]) == (encodeJson res)
Copy link
Owner

Choose a reason for hiding this comment

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

Didn't know you could do that with infixed symbols.

Copy link
Contributor Author

@zhangchiqing zhangchiqing Jan 10, 2017

Choose a reason for hiding this comment

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

I don't like this actually.

res here is a WriteResult, to verify it, It's better to construct another WriteResult then compare. But I can't take this approach, because the constructor of WriteResult is not exported.

That's why I used this workaround here which will encode the WriteResult into Json, then create another Json to compare. I don't like it because it's testing two functions together: insertOne + encodeJson.

What do you think? Is there better pattern for asserting the result?

Copy link
Owner

Choose a reason for hiding this comment

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

Probably is, but let's leave it for now.

@zhangchiqing
Copy link
Contributor Author

Examples have been removed, and Readme.md has been updated

@SimonRichardson could you take a second look?

@SimonRichardson SimonRichardson merged commit 7647641 into SimonRichardson:master Jan 10, 2017
@SimonRichardson
Copy link
Owner

🎉 Congratulations and thanks for the time!

@zhangchiqing
Copy link
Contributor Author

Cool. Thanks for reviewing my PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants