Skip to content

Conversation

@ball6847
Copy link

@ball6847 ball6847 commented Jan 7, 2018

It looks like I'm rewriting your app, because this PR contains a lot of changes 😆

What's changes

  • add --skip-typedefs and its test
  • add test coverage (using istanbul)
  • add missing tests
  • upgrade commander to 2.12 *
  • reformat existing code
  • enhance npm scripts

I need to refactor your code to allow me to test that it can really recognize the newly added option. So, I need a factory function for creating app instance to reuse in test, that's why refactoring is needed. (sorry about that 😅)

*Note about commander

Actually it's not making any difference here (even I upgraded it to 2.12), but while i was working on refactoring I faced typing problem in commander which not allow me to create a factory function. I found a commit on their repository which fix this problem, but unfortunately it's not release yet (but it should soon).

For this reason, the PR contains one hacky workaround and it can be removed when the new commander version is out. You can wait until it's out before merging this PR (I'm fine with that)

The workaround is at src/typings/index.d.ts and only 2 files depend on it.

Coverage

selection_044

Hope this help

@danimbrogno
Copy link
Contributor

Thanks for this.

Can you link me to the issue on the commander repository that you're referring to?

@ball6847
Copy link
Author

ball6847 commented Jan 8, 2018

sure, here it is tj/commander.js#713

@danimbrogno
Copy link
Contributor

Hey @ball6847 , looks like that issue you referenced is merged into commander. Would it be possible to update your PR?

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