Implement Label related methods #1864

su-chang · 2019-10-11T09:20:45Z

I'm submitting a...

[ ] Bug Fix
[x] Feature
[ ] Other (Refactoring, Added tests, Documentation, ...)

Checklist

Commit Messages follow the Conventional Commits pattern
- A feature commit message is prefixed "feat:"
- A bugfix commit message is prefixed "fix:"
Tests for the changes have been added

Description

please describe the changes that you are making

for features, please describe how to use the new feature

please include a reference to an existing issue, if applicable

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

CLAassistant · 2019-10-11T09:20:52Z

All committers have signed the CLA.

su-chang · 2019-10-11T09:22:12Z

related issues:
#1627
#1856

huan

Thank you for your contribution!

The basic design looks great, I think you had understood my design very well after reading your code.

Please follow my reviews, and leave your comments if you want any discussion about this PR.

src/user/tag.ts

…nd implemented all methods relates to Label.

su-chang · 2019-12-02T07:20:27Z

@huan I have implemented all Label related methods conform to design, waiting for your review and suggestions. Thank you.

windmemory · 2019-12-02T07:50:07Z

src/user/tag.ts

+    log.verbose('Tag', 'get()')
+
+    try {
+      const tagPayload: TagPayload = await this.puppet.createTag(tag)


If the tag exists, we should not create a new tag here, right?

It will return the existed tag instead of creating.

Then the function is misleading. We shouldn't call it createTag, it should be getOrCreateTag or something else. createTag explicitly tell the user that this is creating a tag.

Agree, getOrCreateTag() is better for the puppet method.

I guess @huan might not like this design :P

Let's wait for his review.

su-chang · 2019-12-10T11:30:38Z

@huan ping

huan · 2019-12-30T15:09:32Z

I have re-write some code in this PR for designing a more clear & easy to use published API.

Please let me know what you think @su-chang @windmemory @xinbenlv , thank you very much!

The biggest change is that I decided to do not use any payload for Tag class because the tag string itself can be the id of the Tag class.

This design will leave all the implementation details inside the puppet, and we get the highest abstract for our end user API.

windmemory · 2020-01-06T07:11:47Z

src/user/tag.ts

+   * @example
+   * await tag.del()
+   */
+  public async del (from: Contact): Promise<void> {


If we want to delete a tag entirely, how to achieve that with the current design?

We can not do delete tag itself operations in this design.

I think we can just leave tag as they are will be cleaner for the Tag API.

BTW: I think we should rename all del() to remove() because it's more intuitive?

Cleaner Tag API will make Tags in WeChat not clean.

If we don't remove tag which we do not need any more, the tag will make WeChat Tags worse. I think this design will make our user who use tag frequently feel not well.

remove() is better for this case.

windmemory · 2020-01-06T07:11:58Z

src/user/tag.ts

+    tag: string,
+  ): Promise<T['prototype']> {
+    log.verbose('Tag', 'get(%s)', tag)
+    return this.load(tag)


In the get function, there is no actual puppet call, then when shall we create the tag?

In my current design, I want to use Lazy Operation for the Tag.

in the get() we only save the tag string in the class. The actual operation will happen when we call add() or del(), and we pass the tag string to the puppet API, then puppet API deal with the tag name with the Contact or Favorite.

I think this will be a clear design with the high-level API, please feel free to let me know what you think.

Lazy Operation is perfect.

So we could use this method like this way :

const tag = await bot.Tag.get('myTag') const friend =bot.Contact.load(xxx) await tag.add(friend) // we will create a new tag named myTag in this method

So we need to check the tag exist or not each time when we use add()?

I have a question about it, how could we know the tag exist or not ? Or just create new tag each time when we use add()?

This design will make tagAddContact() a little heavy, it means create() + add(), if something wrong happened in create(), it will wrong many times.

BTW, this method should check the tag's length, if user input a string which length over 15, we can not give a right tag back to user. It will be exposed when they use add() method.

su-chang · 2020-01-06T08:38:19Z

I have see these methods only maintain the relationship between Tag and Contact, but no method work for Tag.

For example, if we want to remove one tag from tag list, we need delete method, so that we could never see this tag again in our WeChat App. If we do not have this method, the tags in our WeChat App will be mess really, and so does modify.

huan · 2020-01-08T10:34:29Z

I'll consider adding a delete() method to clean the Tag for the system
I'll assume that the Tag length will not longer than 15 characters. (in UTF8)
I believe it will be better to let the puppet to decide if they need to create a tag before using it.

Any suggestions?

su-chang · 2020-01-08T10:47:05Z

I'd love to the Lazy Operation, and delete() is great for Tag system.

Do we need a method which named modify()?

It has been supported by the WeChat Protocol, and it will make the Tag system be better. But not must.

I'm not insist on it. Just a suggestion.

huan

I have added all required methods for the Tag class, please have a look into it, and please feel free to let me know if you have any questions.

huan · 2020-01-16T05:50:42Z

The Tag class will be available from Wechaty v0.29.41 or later.

su-chang changed the title ~~feat: implement Label related methods~~ Implement Label related methods Oct 11, 2019

This comment has been minimized.

Sign in to view

huan requested a review from windmemory October 12, 2019 07:25

huan requested changes Oct 12, 2019

View reviewed changes

src/user/tag.ts Outdated Show resolved Hide resolved

src/user/tag.ts Outdated Show resolved Hide resolved

su-chang added 6 commits October 31, 2019 14:05

0.29.3

22df662

modify: resolve conflict

888bfe5

Merge branch 'Chatie-master'

4a717c7

modify: label relate methods

b422896

Merge branch 'master' into label

0f2f304

modify: label method

0238bb4

su-chang force-pushed the label branch from ac9e563 to 0238bb4 Compare October 31, 2019 13:19

su-chang added 6 commits November 1, 2019 09:10

Merge branch 'master' into label

5266fa4

modify: make tag class be better.

b37a66c

modify: change return type of method tags() from string[] to Tag[], a…

fd746fa

…nd implemented all methods relates to Label.

modify: bump version

aea6b11

merge: merge origin label

8a7612d

Merge branch 'master' into label

2d139cf

windmemory reviewed Dec 2, 2019

View reviewed changes

su-chang added 2 commits December 2, 2019 16:30

modify: change createTag() to getOrCreateTag()

1b4988d

Merge branch 'label' of https://github.com/su-chang/wechaty into label

8831d39

su-chang requested a review from huan December 5, 2019 02:00

Merge branch 'master' into label

6b9a799

Refactor Tag Design

a006e6b

Merge branch 'master' into label

32951d3

windmemory reviewed Jan 6, 2020

View reviewed changes

huan added 5 commits January 16, 2020 00:30

Merge branch 'master' into label

bcca14e

Refine Tag design

96c1554

0.29.41

be2d0ae

remove findAll for Tag class

cc6acbf

add todo for check tag before delete

811f6c5

huan approved these changes Jan 16, 2020

View reviewed changes

huan merged commit ea604bf into wechaty:master Jan 16, 2020

huan mentioned this pull request Jan 16, 2020

Implement Label in Wechaty #1856

Closed

Uh oh!

Implement Label related methods #1864

Implement Label related methods #1864

Uh oh!

Conversation

su-chang commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I'm submitting a...

Checklist

Description

Does this PR introduce a breaking change?

Uh oh!

CLAassistant commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

su-chang commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

huan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

su-chang commented Dec 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

su-chang commented Dec 10, 2019

Uh oh!

huan commented Dec 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

su-chang commented Jan 6, 2020

Uh oh!

huan commented Jan 8, 2020

Uh oh!

su-chang commented Jan 8, 2020

Uh oh!

huan left a comment

Choose a reason for hiding this comment

Uh oh!

huan commented Jan 16, 2020

Uh oh!

Uh oh!

su-chang commented Oct 11, 2019 •

edited

Loading

CLAassistant commented Oct 11, 2019 •

edited

Loading

su-chang commented Oct 11, 2019 •

edited

Loading

huan commented Dec 30, 2019 •

edited

Loading