Skip to content
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

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

Closed
wants to merge 26 commits into from
Closed

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

wants to merge 26 commits into from

Conversation

shiftkey
Copy link
Contributor

@shiftkey shiftkey commented Jan 4, 2017

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
Copy link
Contributor Author

shiftkey commented Jan 4, 2017

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

if (typeof options === 'function') {
callback = options
options = null
function getText(stdout: Buffer | string): string {
Copy link
Contributor Author

@shiftkey shiftkey Jan 4, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

shiftkey commented Jan 5, 2017

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

@jlord
Copy link
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
Copy link
Contributor Author

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 add-flow-support branch January 5, 2017 23:23
@shiftkey shiftkey mentioned this pull request Jan 7, 2017
4 tasks
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.

2 participants