node-wit icon indicating copy to clipboard operation
node-wit copied to clipboard

Adds support for msg_id & thread_id

Open Xotl opened this issue 6 years ago • 5 comments

Fixes issue #189, #191 & #169.

Xotl avatar May 27 '19 02:05 Xotl

Can you pass in the new params directly instead? The current change may be a bit misleading, and we can pass in default params for threadId and msgId to get around the backwards compatibility issue

jtliao avatar May 30 '19 14:05 jtliao

Having Wit.message() to receive more params it's not maintainable. It just increases the Cyclomatic complexity.

A good approach to solve this problem is to have an object with the params we want.

See issue #189.

Xotl avatar Jun 03 '19 18:06 Xotl

I would argue the complexity is higher if a single argument takes different forms. I'm not sure how adding two params with default values makes thing not maintainable.

jtliao avatar Jun 04 '19 15:06 jtliao

if we pass a single object we can just iterate over the properties of such object instead of adding more if's.

Regarding the maintainability, what will happen if suddenly the /messages endpoint deprecates one field?, let's say verbose gets deprecated... you will be passing null always for that parameter even though it's no longer used.

Xotl avatar Jun 04 '19 17:06 Xotl

Updated the PR to show what the implementation look like

Xotl avatar Jun 05 '19 02:06 Xotl