search

vilicvane / clime

⌨ The command-line interface framework for TypeScript.
252 stars 10 forks source link

Support `ts-node` by checking `.ts` source files #47

Open marshall007 opened 6 years ago

marshall007 commented 6 years ago

Should fix #41, but I haven't been able to test yet.

I still have a few more cases to update like in help.ts#L108-L117, but I wanted to run this PR by you before continuing (mostly because I'm unsure how we should go about adding tests for this).

For what it's worth, I also don't think this is a good long-term solution. I would really like to see #43 implemented, but this seemed like the easier path forward for now to get my feet wet.

marshall007 commented 6 years ago

Note the build failures already existed on master and are not a result of this change.

vilicvane commented 6 years ago

Hi @marshall007 , thanks for this PR. Yes the CI failure was caused by an accidentally committed file by me.

Along with other feature requests, I think there would be a major refactoring for clime in the near future.

And this is PR is has some intersections with #42 , which addresses this issue in a configurable approach. I am actually okay with both, so it would be nice if you may update the correspondent part of help.ts.

As for tests, you can check out https://github.com/vilic/clime/blob/master/src/test/baseman/cli-test.ts#L45

I think maybe we can simply duplicate all .js tests for .ts by generating both of them.

marshall007 commented 6 years ago

@vilic I just pushed up another commit addressing the remaining cases in help.ts. Could still use some direction on testing. I can't really tell how the generator works. Perhaps it would be easier to just run baseman with ts-node prior to compiling the TS test source files?

vilicvane commented 6 years ago

@marshall007 You might be right. We need some efforts on baseman implementation though. I will make necessary changes to baseman and test case generating here, and once done will merge your PR (if it passes). Thanks!

marshall007 commented 6 years ago

@vilic no problem, just let me know if you need anything else! Cheers!

marshall007 commented 6 years ago

@ajafff good call on using require.resolve, just updated this PR to do so. Note I rebased on top of 0e075f1 to get rid of those test failures. The existing (JS-based) tests still passed on my local using the new require.resolve logic.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.007%) to 97.578% when pulling 4ef9e9a5cc89a16566199aeb65255623862b8f42 on marshall007:marshall_41 into 0e075f13cf2b529860fa67d7b06ed1cf091451ff on vilic:master.