spring clean #31

davidchambers · 2017-05-10T08:54:59Z

It's been a while since anyone gave this project love and attention, so here's some ❤️.

Changes:

Replace Ramda with Sanctuary
Replace data.task with Fluture
Define common internal functions: exit0, exit1, join, and readFile
Remove --harmony flags
Remove Babel, which is no longer required now that Node supports await
Upgrade dependencies
Integrate ESLint and sanctuary-style

davidchambers · 2017-05-10T08:57:29Z

bluebird-promisell.js

-          process.stderr.write(String(err) + '\n');
-          process.exit(1);
-        });
+  concatFiles(join(process.argv[2])).then(exit0, exit1);
 };

 if (process.mainModule.filename === __filename) main();


You may be interested in these changes, @zhangchiqing.

Nice refactor!

For the concatFiles function, it's very clean to chain all the sub functions together. However I'm afraid it would be a bit hard for learners to read.

I'd like to add some intermediate variables and type signatures so that it's clear what each step returns.
And this style also highlights that the logic reads as if it's imperative and sync, but is actually functional and async.

// concatFiles :: (String -> String) -> Promise Error String const concatFiles = path => { // String -> Promise Error String const readFileFromPath = S.compose(readFile, path); // Promise Error String const indexFileP = readFileFromPath('index.txt'); // Promise Error [String] const fileNamesP = P.liftp1(S.lines)(indexFileP); // Promise Error [String] const filesP = P.liftp1(P.traversep(readFileFromPath)(fileNamesP); // Promise Error String return P.liftp1(S.joinWith(''))(filesP); }

Thanks for the feedback, @zhangchiqing. I pushed an update. Let me know what you think of it. :)

Looks great! Thanks @davidchambers 👍

davidchambers · 2017-05-10T08:59:28Z

futures.js

+  concatFiles(join(process.argv[2])).fork(exit1, exit0);
+};
+
+if (process.mainModule.filename === __filename) main();


Does this file look good to you, @Avaq?

davidchambers · 2017-05-10T09:01:19Z

kgo.js

+  ('files', ['filePaths'], S.curry3(foreign.parallel, readFile))
+  ('concated', ['files'], kgo.sync(S.joinWith('')))
+  (['concated'], exit0)
+  (['*'], exit1);
 };

 if (process.mainModule.filename === __filename) main();


You may be interested in these changes, @KoryNunn.

Honestly I'm not advocating for kgo's usage these days, but rather suggest people look at righto. I'd be happy to drop this case.

Okay. Done. :)

davidchambers · 2017-05-10T09:04:07Z

lazy-either.js

+//
+//    We cannot use S.traverse because LazyEither does not currently support
+//    recent versions of the Fantasy Land specification. It's thus necessary
+//    to dispatch to `map` and `ap` (rather than their prefixed equivalents).


Are you still maintaining LazyEither, @Risto-Stevcev? If so, have you plans to support fantasy-land/-prefixed methods?

It's implementing the fantasy land spec, what are the fantasy-land prefixed methods?

It's not actively developed but it's lazily maintained (accepting PRs). Avaq's fluture lib handles the basic cases for using Either and Maybe with futures so I recommend that to people. Due to LazyEither's restrictions, it does have some interesting properties that fluture can't have because it's more general, but I'm more interested in purescript right now.

It's implementing the fantasy land spec, what are the fantasy-land prefixed methods?

Methods have required the fantasy-land/ prefix since fantasyland/fantasy-land#146 which was released in v1.0.0. The current version is v3.2.0, so LazyEither is a little out of date. I'll submit a pull request to prefix the methods. :)

Risto-Stevcev/lazy-either#1

I'll remove the LazyEither solution, Risto. As I stated in Risto-Stevcev/lazy-either#2, Fluture and LazyEither are duplicative. There's little value in including both here.

davidchambers · 2017-05-10T09:06:08Z

most.js

-    process.stderr.write(String(err) + '\n');
-    process.exit(1);
-  });
+  concatFiles(join(process.argv[2])).then(exit0, exit1);
 };

 if (process.mainModule.filename === __filename) main();


You may be interested in these changes, @Risto-Stevcev.

davidchambers · 2017-05-10T09:08:09Z

promises-pipe.js

+  concatFiles(join(process.argv[2])).then(exit0, exit1);
+};
+
+if (process.mainModule.filename === __filename) main();


You may be interested in these changes, @paldepind.

Nice 😄 Why did you change then(Promise.all) to then(Promise.all.bind(Promise))? It should work fine without the bind I think.

I switched from Bluebird promises to native promises, which require context binding.

davidchambers · 2017-05-10T09:09:25Z

righto.js

-      process.exit(0);
-    }
+  concatFiles(join(process.argv[2]))((err, data) => {
+    if (err == null) exit0(data); else exit1(err);
  });
 };



You may be interested in these changes, @KoryNunn.

The change to remove variables for each logical step goes violently against the grain of righto, and makes this, to my eyes, unreadable.

Also the change to use sanctuary within this example in my opinion muddies it, since a new viewer would have to look up how it works AND how righto works.

At this point, since all the examples use sanctuary, I don't see this as a useful comparison, but a contrived example that is designed explicitly to show of one or two particular examples.

I updated the implementation. Is this more to your liking?

// concatFiles :: (String -> String) -> Righto String const concatFiles = path => { const readFileRel = S.compose(readFile, path); const index = readFileRel('index.txt'); const files = righto.sync(S.compose(S.map(readFileRel), S.lines), index); return righto.sync(S.joinWith(''), righto.all(files)); };

That's definitely much more like how it is intended to be used.

Okay. Thank you for your feedback, Kory. :)

Avaq · 2017-05-10T10:14:13Z

futures.js

+          readFile,                               // :: Future Error String
+          S.map(S.lines),                         // :: Future Error (Array String)
+          S.map(S.map(path)),                     // :: Future Error (Array String)
+          S.chain(S.traverse(Future, readFile)),  // :: Future Error (Array String)


If it's important to read all files in parallel, we can replace this line with one of two versions:

Using traverse over ConcurrentFuture (Par) structure:

S.chain(S.compose(Future.seq, S.traverse(Future.Par, S.compose(Future.Par, readFile))))

Using Future.parallel:

S.map(S.map(readFile)), S.chain(Future.parallel(Infinity))

Reading files in parallel would be very nice indeed! Which version do you favour, and why? I'm happy to follow your recommendation.

It saddens me* to prefer the second version, because it's more concise and it's clearer what's going on. It's also more similar to many of the Promise based solutions in the project.

* It would be nicer to be able to utilize traverse for this purpose without the verbose type juggling. If we would go for the first approach, some ways to make it more readable would be to name S.compose(Future.Par, readFile) something like concurrentReadFile, and maybe use map to avoid the nested compose:

const concurrentReadFile = S.compose(Future.Par, readFile); //... S.map(S.traverse(Future.Par, concurrentReadFile)), // :: Future Error (ConcurrentFuture Error (Array String)) S.chain(Future.seq) // :: Future Error (Array String)

I'll leave it up to you to decide which of the three versions you deem most appropriate. I can't decide.

davidchambers · 2017-05-10T22:19:01Z

common/sanctuary.js

+  x => S.type(x) === Future.Par['@@type'],
+  f => [],
+  f => []
+);


Does this look right to you, @Avaq? It seems the ConcurrentFuture type lacks helpers corresponding to those that exist for defining FutureType :: Type -> Type -> Type. I'd love to be able to do this:

const Future = require('fluture'); const $ = require('sanctuary-def'); const {ConcurrentFutureType, FutureType} = Future.defineTypes($);

Fluture could then provide type definitions without depending on sanctuary-def (although it would be a good idea to specify sanctuary-def as a peer dependency). What do you think?

I'd love to be able to [use something like Future.defineTypes($)]

fluture-js/Fluture#101

Avaq · 2017-05-11T09:14:15Z

common/sanctuary.js

+  'https://github.com/fluture-js/Fluture#concurrentfuture',
+  x => S.type(x) === Future.Par['@@type'],
+  f => [],
+  f => []


I suppose you could use compose(Future.extractLeft, Future.seq).

Avaq · 2017-05-16T13:13:16Z

futures.js

@@ -12,7 +12,7 @@ const S             = require('./common/sanctuary');

 //    readFile :: String -> Future Error String
 const readFile =
-S.compose(Future.node, S.curry3(fs.readFile, S.__, {encoding: 'utf8'}));
+S.curry3(Future.encaseN2, fs.readFile, S.__, {encoding: 'utf8'});


I prefer the more concise S.flip(Future.encaseN2(fs.readFile), {encoding: 'utf8'}), why did you choose this option?

I find the placeholder clearer than flipping in this case. I'm happy to change it, though.

Avaq · 2017-05-16T16:44:18Z

await.js

+  Promise.all(S.map(readFileRel, S.lines(index)))
+  .then(S.joinWith(''))
+  .then(exit0, exit1);
+}


Shouldn't this file be structured the same way the coroutine-files are?

// concatFiles :: (String -> String) -> Promise Error String async function concatFiles(path){ const index = await readFile(path('index.txt')); const filenames = S.map(path, S.lines(index)); const results = await Promise.all(S.map(readFile, filenames)); return S.joinWith('', results); } const main = () => { concatFiles(join(process.argv[2])).then(exit0, exit1); }

Avaq · 2017-05-16T17:24:09Z

most.js

+  .map(S.map(path))
+  .map(S.map(readFile))
+  .map(Promise.all.bind(Promise))
+  .awaitPromises()


I think it's weird to use Promise.all followed by awaitPromises to await a single Promise in the context of streaming.

We could use most.from to create a stream of multiple promises, and then await all of them. That also avoids the joining of an array of strings followed by the joining of a single string to an empty string.

// concatFiles :: String -> Promise Error String const concatFiles = path => most.fromPromise(readFile(path('index.txt'))) // :: Stream Error String .map(S.lines) // :: Stream Error (Array String) .map(S.map(path)) // :: Stream Error (Array String) .map(S.map(readFile)) // :: Stream Error (Array (Promise Error String)) .chain(most.from) // :: Stream Error (Promise Error String) .awaitPromises() // :: Stream Error String .reduce(S.joinWith(''), '') // :: Promise Error String

This seems much better (although the reducing function should be S.concat rather than S.joinWith('')). When I try it in one of the failure cases, though, I see:

UnhandledPromiseRejectionWarning: Unhandled promise rejection

DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Ah yes - I got confused. I think S.joinWith('') is equal to reduce(S.concat, '').

Do you know which Promise is failing? The index.txt or one of the others?

Do you know which Promise is failing?

No. Determining this will require investigation.

The problem is that the Promise settles before Most attaches handlers. In order to make sure handlers are attached, we use most.fromPromise immediately on the return value from readFile:

// readFileStream :: String -> Stream Error String const readFileStream = S.compose(most.fromPromise, readFile); // concatFiles :: (String -> String) -> Promise Error String const concatFiles = path => most.fromPromise(readFile(path('index.txt'))) // :: Stream Error String .map(S.lines) // :: Stream Error (Array String) .map(S.map(path)) // :: Stream Error (Array String) .map(S.map(readFileStream)) // :: Stream Error (Array (Stream Error String)) .chain(most.mergeArray) // :: Stream Error String .reduce(S.concat, '') // :: Promise Error String

This version handles the failure cases but no longer guarantees order in the success case. I'd love to see an updated version, Aldwin. :)

Let's keep the solution as is, I'm no frequent most.js user and cannot easily solve the "Promise settles before Most attaches handlers" problem. It's essentially just another Promise flaw - running into these issues really makes me appreciate Fluture.

Avaq

I think these changes are an overall improvement in their current form. I will have a quick look into the most.js solution.

EDIT: All tests are passing on this branch. The UnhandledPromiseRejectionWarning only occurs on master.

EDIT2: I tested with Node 6 and Node 4 as well as Node 7, for good measure. The most.js tests are passing in all versions.

EDIT3: Ignore all of the above. Here's the fix

davidchambers · 2017-06-21T13:32:36Z

I'm about to squash the commits. The original commits can be found at e8e5d4f.

davidchambers commented May 10, 2017

View reviewed changes

davidchambers requested a review from CrisFeo May 10, 2017 09:10

Avaq reviewed May 10, 2017

View reviewed changes

davidchambers commented May 10, 2017

View reviewed changes

Avaq reviewed May 11, 2017

View reviewed changes

davidchambers mentioned this pull request May 13, 2017

support recent versions of the Fantasy Land specification Risto-Stevcev/lazy-either#1

Merged

Avaq reviewed May 16, 2017

View reviewed changes

Avaq approved these changes May 16, 2017

View reviewed changes

Avaq reviewed May 16, 2017

View reviewed changes

zhangchiqing approved these changes May 17, 2017

View reviewed changes

Avaq approved these changes May 31, 2017

View reviewed changes

Avaq approved these changes Jun 21, 2017

View reviewed changes

spring clean

b4986db

davidchambers force-pushed the davidchambers/spring-clean branch from e8e5d4f to b4986db Compare June 21, 2017 13:33

davidchambers merged commit 0ea31b0 into master Jun 21, 2017

davidchambers deleted the davidchambers/spring-clean branch June 21, 2017 13:37

spring clean #31

spring clean #31

Conversation

davidchambers commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Avaq left a comment • edited Loading

Choose a reason for hiding this comment

davidchambers commented Jun 21, 2017

Avaq May 10, 2017 •

edited

Loading

Avaq May 10, 2017 •

edited

Loading

Avaq May 16, 2017 •

edited

Loading

Avaq May 16, 2017 •

edited

Loading

Avaq May 31, 2017 •

edited

Loading

Avaq Jun 1, 2017 •

edited

Loading

Avaq left a comment •

edited

Loading