[EXPERIMENT] add support for checking types with Flow #178

Closed
wants to merge 26 commits into
from

Projects

None yet

2 participants

@shiftkey
Contributor
shiftkey commented Jan 4, 2017 edited

I'm not sure if this is appealing to you, but as part of my investigation into 🔥ing the PortableGit dependency I started down the path of what it would take to get flow incorporated (so I don't break the callers in the app).

TL;DR:

  • I now have flow testing the spawnGit method and where it's being used
  • I made the method signature more explicit rather than get too deep into flow to handle the magic of JS. I plan to come back to that.
  • I added electron-compile to handle the transpiling which needs to remove the flow type information, so that we don't need to worry about changing the packaging steps
  • standard now breaks when testing these annotations - I could swap this out to use the Node API and pass in the transpiled output, but that would mean a standalone script rather than a simple command

Ship List:

  • annotate spawnGit and it's callers to verify the type information
  • update app so that it strips type information at runtime
  • run tests for flow and standard as part of npm test
  • cleanup existing usages of spawnGit to drop unnecessary execOpts parameters
@shiftkey
Contributor
shiftkey commented Jan 4, 2017

Oh, and using flow was how I found #175 😁

lib/spawn-git.js
- if (typeof options === 'function') {
- callback = options
- options = null
+function getText(stdout: Buffer | string): string {
@shiftkey
shiftkey Jan 4, 2017 edited Contributor

This code feels really 💩 but I haven't really dug into whether it's correct here to be paranoid about exec returning a Buffer for stdout and stderr

@shiftkey
Contributor
shiftkey commented Jan 5, 2017

Let me know if you'd like the Travis changes extracted to a different PR

@jlord
Owner
jlord commented Jan 5, 2017

Oh dang, I hate that you did all this work! In general I'm anti things that you have to precompile so I wouldn't want to to add Flow or Babel. I'd rather have tests help find errors like the one with request. There are no tests in this project 😱 I'd like to have simple tests with tape/tap to tests the verify scripts but that always gets pushed to the bottom of my list.

I am, however, very interested in 🔥 or reducing Portable Git, though I'm not sure how Flow/Babel would help with that? Can you explain more about how you were thinking it could be 🔥 'd?

@shiftkey
Contributor
shiftkey commented Jan 5, 2017

@jlord don't sweat it - it was a fun learning experience for me.

There are no tests in this project 😱 I'd like to have simple tests with tape/tap to tests the verify scripts but that always gets pushed to the bottom of my list.

That's fine. I'll have a look into where I could help out with that.

I am, however, very interested in 🔥 or reducing Portable Git, though I'm not sure how Flow/Babel would help with that? Can you explain more about how you were thinking it could be 🔥 'd?

The project itself uses Typescript and removes the dependency of having Git on your PATH - it also supports Windows/macOS/Ubuntu, so it feels like a great fit for Git-It. I wanted to use this exercise as a way to catch regressions as I went through the refactoring exercise, but given how specific spawnGit is I think I can live without needing Flow/Babel.

@shiftkey shiftkey closed this Jan 5, 2017
@shiftkey shiftkey deleted the shiftkey:add-flow-support branch Jan 5, 2017
@shiftkey shiftkey referenced this pull request Jan 7, 2017
Open

[WIP] A Proposal For Testing Things #180

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment