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

refactor: add typing to all non-generated functions of speech-to-text #954

Merged
merged 2 commits into from
Sep 16, 2019

Conversation

MasterOdin
Copy link
Contributor

This adds typing for the three functions of ./speech-to-text/v1.ts that lacked them, as well as removes any implicit definitions of any in the file.

I decided it would be more pragmatic to make RecognizeStream.Options a superset of Duplex.Options and SpeechToTextV1.RecognizeWebSocketParams and not duplicate the definitions across the two files, especially given the stream is only used for that particular method in the SDK itself.

I also added the optional bit to all optional properties of RecognizeStream.Options as part of this.

I used extend a couple of times (as it was already in the project and in this file) so that I wouldn't have to make (for example) WhenCorporaAnalyzedOptions.errorFilter "optional" to allow it to be defined on the next line. I didn't do that with SpeechToTextError as it's not possible to define a second class in the file per the tslint and so had to make due with a weaker interface design with optional code even though it should never actually be undefined in usage.

Checklist
  • npm test passes (tip: npm run autofix can correct most style issues)

@codecov-io
Copy link

codecov-io commented Sep 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (release-candidate-v5@600f9e0). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##             release-candidate-v5     #954   +/-   ##
=======================================================
  Coverage                        ?   64.28%           
=======================================================
  Files                           ?        2           
  Lines                           ?       14           
  Branches                        ?        3           
=======================================================
  Hits                            ?        9           
  Misses                          ?        4           
  Partials                        ?        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 600f9e0...bee7b18. Read the comment docs.

@MasterOdin
Copy link
Contributor Author

Codacy quality review is on adding the namespace RecognizeStream and namespace SpeechToTextV1, not sure if there was a preferred new method yet on how to do this typing as I believe all other classes still use this method.

@dpopp07
Copy link
Contributor

dpopp07 commented Sep 13, 2019

@MasterOdin I actually couldn't care less about what Codacy says, a lot of times it even contradicts our own style/linting guides. That said, I was going to comment on the introduction of namespaces here since I believe they are generally recommended against and I've been thinking about trying to refactor how we organize the other files to remove them.

Now I see why you would want them for the STT class so that it's consistent for the other services but for the RecognizeStream class, would it make a big difference to do something like this?

export interface RecognizeStreamOptions extends DuplexOptions, SpeechToTextV1.Options {
  // these options represent the superset of the base params,
  // query params, and opening message params, with the keys
  // in lowerCamelCase format so we can expose a consistent style
  // to the user.
  authenticator: Authenticator;
  url?: string;
}

@MasterOdin
Copy link
Contributor Author

MasterOdin commented Sep 13, 2019

Unfortunately, RecognizeStream, like all the other classes, does at the bottom:

export = RecognizeStream;

which essentially binds this to using the namespace method. You could refactor it so that both the RecognizeStream and RecognizeStreamOptions are top level exports, but I think runs into the stuff I talk about in #897 (comment). Might be better to handle this class at the same time as all the others so that they all share the same structure? I do agree in getting rid of the namespaces at some point though to be following TS best practices, and would be happy to help on that front / mock up what a preferred approach might look like.

Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

This all looks pretty good. Just had one question but otherwise I'm ready to approve it

lib/recognize-stream.ts Show resolved Hide resolved
lib/recognize-stream.ts Outdated Show resolved Hide resolved
speech-to-text/v1.ts Show resolved Hide resolved
@dpopp07
Copy link
Contributor

dpopp07 commented Sep 13, 2019

Might be better to handle this class at the same time as all the others so that they all share the same structure?

That's a good point, we can do that.

I do agree in getting rid of the namespaces at some point though to be following TS best practices, and would be happy to help on that front / mock up what a preferred approach might look like.

That's something I've been wanting to get to recently but just have not had the time. If you want to take a stab at the design for that, that would be great and I'd be happy to review it with you. I think the big limitation there is that we are pretty ingrained with the import naming structure for the SDK (ibm-watson/speech-to-text/v1, etc.) but we should think about the possibilities.

Another thing to think about is that if it would help to define our own .d.ts files (I don't have much experience in that area, specifically) but you think that would be too tedious - we can auto-generate them like we do the rest of code. Just something to be aware of.

@MasterOdin
Copy link
Contributor Author

You'd only want to do the *.d.ts route only because the library was written in JS and that was the only way to get typings for TypeScript to use (it's the basis of @types). I can certainly play around with some options after I've knocked these next two things I was planning on doing out:

  • updated husky and commitlint to get rid of the last 7 npm audits (ran into a snag as tsc suddenly started complaining about a bunch of errors, but I think that was just a weird node_modules installation)
  • added typings for text-to-speech/v1

Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks again for the PR!

@dpopp07 dpopp07 merged commit a73a747 into watson-developer-cloud:release-candidate-v5 Sep 16, 2019
@MasterOdin MasterOdin deleted the stt_typing2 branch September 27, 2019 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants