actions-on-google / actions-on-google-testing-nodejs

Apache License 2.0
75 stars 18 forks source link

Library Alpha Discussion: Is the API too verbose? #8

Closed Fleker closed 6 years ago

Fleker commented 6 years ago

Hey AoG Devs!

Now that the alpha version of the testing library has been out for a few weeks, we'd like to get feedback on the behavior and API design of the library.

The API contains several instances of methods that concatenate more than one word. action.setLocale action.startConversation action.startConversationWith action.endConversation

For most APIs, having simple function names are preferable. In testing libraries, APIs tend to be slightly more verbose. Should it make sense to simplify the set of APIs so that they’d be more like:

action.locale = ‘en-US’ action.start() action.start(‘number genie’) action.cancel()

AlexBravo commented 6 years ago

IMHO the first, more verbose version of methods is fine. Being too short can actually make things harder to read.

yoichiro commented 6 years ago

Is it difficult to support both? If yes, I think that simple is better. +1 against the latter (locale = ... start() cancel()).

Fleker commented 6 years ago

Even if we do support aliasing of methods, ie. action.start == action.startConversation, which would be preferable in recommended usage?

yoichiro commented 6 years ago

Even if we do support aliasing of methods, ie. action.start == action.startConversation, which would be preferable in recommended usage?

@Fleker Probably, we should use the prototype chain. For example, we can define each method alias by using the interface which has the same name:

...
export class ActionsOnGoogle {
    ...
}

export interface ActionsOnGoogle {
    start: typeof ActionsOnGoogle.prototype.startConversation
    startWith: typeof ActionsOnGoogle.prototype.startConversationWith
    cancel: typeof ActionsOnGoogle.prototype.endConversation
}

ActionsOnGoogle.prototype.start = ActionsOnGoogle.prototype.startConversation
ActionsOnGoogle.prototype.startWith = ActionsOnGoogle.prototype.startConversationWith
ActionsOnGoogle.prototype.cancel = ActionsOnGoogle.prototype.endConversation

But, it seems that this approach has a weak point for documentation. If we want to create an API reference with the typedoc, I guess that we should simply add each aliased method to the ActionsOnGoogle class, so that we can see the added alias methods as same as existed methods on the ActionsOnGoogle class doc.

Also, we can support both the setLocale() function and a new locale property by like the following code:

...
export class ActionsOnGoogle {
    ...
    // Current setLocale method
    setLocale(l: string) {
        if (SUPPORTED_LOCALES.concat(Object.keys(FALLBACK_LOCALES)).indexOf(l) === -1) {
            console.warn(`Warning: Unsupported locale '${l}' in this tool. Ignore.`)
        }
        this._locale = l
        i18n.setLocale(l)
    }

    // Can add a `locale` property with the setter below
    set locale(l: string) {
        this.setLocale(l)
    }
    ...
yoichiro commented 6 years ago

@Fleker I created the pull request #11 to add the aliases and locale property. Of course, we should collect more opinions for this theme. And, you can reject the pull request depending on a result of the discussion here. :)