Open pbalaram opened 7 years ago
Thanks for the PR! This looks great, but I think we don't need classes. I'd rather move to using typescript and interfaces. That way we can get all the es7+ features and avoid classes
Sure, TypeScript makes sense. I've converted the core library to use it. I'm not sure why we would avoid classes, would you mind explaining a bit further?
Frankly I could go either way on this, but @ashishsc has strong feeling about code style that I typically defer to. Regardless, the one change I would request is that you include a prepublish
script in package.json
using
npm run build
That way I never accidentally publish the npm package without building the library first :)
Thanks for converting to typescript! we <3 you.
As far as I why I don't prefer classes. I like being able to pass the sonus object in as an easy way to mock the methods. Also this will be a breaking change.
However, I come from a functional programming background (internal state makes things harder to test, not that we have any) but @evancohen has more object-oriented experience so since he's fine with classes, by all means we should go with that.
Thank you for the feedback!
@evancohen I've added a prepublish
step, which should call npm run build
.
@ashishsc I've added tslint, fixed the trivial warnings, and added rules for the more widespread ones.
I think with the linter setting enabled we can already avoid these issues unless I'm mistaken?
On Sun, Mar 26, 2017, 7:30 PM Prithvi Balaram notifications@github.com wrote:
@pbalaram commented on this pull request.
In src/sonus.ts https://github.com/evancohen/sonus/pull/35#discussion_r108082426:
- threshold: 0,
- device: this._device || null,
- recordProgram: this._recordProgram || "rec",
- verbose: false
- });
- this._mic.pipe(this._detector);
- this._started = true;
- }
- public stop() {
- stopRecording();
- }
- public pause() {
- this._mic.pause();
Correct! They did resolve this particular issue, my intent was simply to suggest that we might be able to avoid this class of issues entirely by keeping semi-colons mandatory.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/evancohen/sonus/pull/35#discussion_r108082426, or mute the thread https://github.com/notifications/unsubscribe-auth/ACgRCtNWQm3pci1KjIvo9dH5rATC-GLsks5rpx80gaJpZM4MMSst .
Hey folks! I want to get this merged, but things were left in a bit of an ambiguous state. @ashishsc are we good on the linter settings?
I'm good with what we have currently, we can always change it later. We should just use an autoformatter and reduce bikeshedding at some point. Once pretter finishes their TS support, I'll add that in here.
With these changes, we can now use ES7 features.
I figure this might help keep things organized, as the codebase grows.
Bonus: You can now run examples/example.js by running the following.