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

Apache License 2.0
75 stars 18 forks source link

Support simple API names and add `locale` property #11

Closed yoichiro closed 6 years ago

yoichiro commented 6 years ago

This pull request will fix #8.

What is this pull request

In this current version, the names of APIs tend to be verbose. Especially, they are the followings:

In this pull request, these aliases are provided. That is, the following APIs are provided:

How to implement them

For adding the locale property, I will add a new definition of the locale setter method into the ActionsOnGoogle class.

For adding the alias methods, I will add the alias method definitions to the prototype of the ActionsOnGoogle class.

yoichiro commented 6 years ago

@Canain I have changed the alias approach to adding methods simply. I would like you to check the following:

Canain commented 6 years ago

@Fleker can you take a look and see if it works for you?

Fleker commented 6 years ago

I do like the more concise methods, although I am hesitant to start creating a bunch of aliases for methods before we even get to a v1 as it'll end up cluttering the API.

Maybe we should use these new methods as the default way and deprecate the current versions. We can accept the aliasing for now, with a plan to remove those verbose method names in a v1 release.

yoichiro commented 6 years ago

@Fleker @Canain Thank you for the confirmation. Ok, I intend to change my code to the following:

I would like you to review again after I change and commit them.

yoichiro commented 6 years ago

@Fleker @Canain Could you review the following commits?

yoichiro commented 6 years ago

@Fleker I have just changed codes you mentioned. Could you review diffs 3086897 5d3dbd4 1336a5e again? BTW, I removed method names which are deprecated from the README.md at 1336a5e to navigate to use the new simple methods.

Fleker commented 6 years ago

@atulep what do you think? It looks fine to me.

yoichiro commented 6 years ago

@atulep Could you confirm this?

yoichiro commented 6 years ago

@atulep Thank you for the reviewing. Both answers of the questions are yes. I do them.

yoichiro commented 6 years ago

@atulep Could you review their changes below?

yoichiro commented 6 years ago

@atulep Thank you for the reviewing!

yoichiro commented 6 years ago

@Fleker @Canain Could you do this final judgement?

Fleker commented 6 years ago

Yeah this looks good to me.

Canain commented 6 years ago

Looks good to me.

yoichiro commented 6 years ago

@Canain @Fleker @atulep Thanks, guys. Can somebody merge this to the master branch? :)

Fleker commented 6 years ago

I ran the tests and everything is good. We'll do the merge and release on Monday, so as not to break things over the weekend. :grin:

yoichiro commented 6 years ago

@Fleker Ok, have an enjoy your weekend!