Skip to content
This repository was archived by the owner on Feb 18, 2022. It is now read-only.

Conversation

zhangchiqing
Copy link
Contributor

The concatFiles function is still "big" to me.

In this PR,

  1. I'm using bluebird-promisell to break the concatFiles function into small ones.
  2. So each small function either play only one side effect, or play multiple side effects of the same type.
  3. I like keeping the logic flat, so that the return value makes it clear what would be the resolved promise value.
  4. The syntax reads like synchronized, but all the promises are chained.

// readFilesWithDirAndNames :: String -> [String] -> Promise Error [String]
const readFilesWithDirAndNames = R.curryN(2, (dir, fileNames) => {
const readOneFile = readFileWithDirAndName(dir);
return P.traversep(readOneFile)(fileNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a pity that with promises we're forced to use specialized functions such as traversep rather than generalized functions such as traverse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree. I wish I could use R.traverse, but Promise is not compatible with it. That's why I made the bluebird-promisell library.

I guess the Task solution can use R.traverse ?

@zhangchiqing
Copy link
Contributor Author

@davidchambers Thanks for your review!

I've fixed all your comments. Please take a second look :)

const UTF8 = makeEncoding('utf8');

// readFileWithDirAndName :: Encoding -> String -> String -> Promise Error String
const readFileWithDirAndName = R.curryN(3, (encoding, dir, name) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use R.curryN(3, ...) rather than R.curry(...) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, actually R.curry is simplier. Updated!

@davidchambers
Copy link
Contributor

I left two more comments, but otherwise this looks great. Could you squash the commits?

@zhangchiqing
Copy link
Contributor Author

Thanks @davidchambers !

I fixed those comments. Could you please take another look :)

const readFile = R.curry((encoding, filename) =>
new Promise((resolve, reject) => {
fs.readFile(filename, {encoding: encoding}, (err, data) => {
if (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use if (err != null) here to avoid type coercion. I'm sorry that I didn't spot this earlier.

@davidchambers
Copy link
Contributor

I just noticed one last thing. git commit --amend and I will merge. :)

@zhangchiqing
Copy link
Contributor Author

👍 Done :)

@davidchambers
Copy link
Contributor

Thanks, @zhangchiqing!

I just realized that I no longer have write access to this repository. @amcbride, could you add me as a collaborator?

@davidchambers
Copy link
Contributor

🌳

@davidchambers davidchambers merged commit 6d8bc21 into plaid:master Aug 1, 2016
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