Skip to content

Add --skip-typedefs option #4

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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