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

add receive and send link prototype #1538

Merged
merged 5 commits into from
Aug 10, 2018
Merged

add receive and send link prototype #1538

merged 5 commits into from
Aug 10, 2018

Conversation

windmemory
Copy link
Member

@windmemory windmemory commented Aug 10, 2018

See #1539

@huan
Copy link
Member

huan commented Aug 10, 2018

Thanks for the PR.

However, please move all the discussion to an issue before you create a PR, and only keep the issue number in the PR content, which I think it's better to show what's happened in the CHANGELOG.

@windmemory
Copy link
Member Author

Got it, will create issue next time.

@huan huan merged commit 27b95f4 into wechaty:master Aug 10, 2018
@huan
Copy link
Member

huan commented Aug 10, 2018

Thanks for the prototyping.

I had added a new class named UrlLink and it should be used for sending/receiving Url Type Messages.

Merged.

@windmemory
Copy link
Member Author

It seems like the current design for UrlLink is that, once the link is created, the content(title, description, url, thumbnail url) can not be modified. Can we make the properties modifiable? I think people might want to have control on that.

@huan
Copy link
Member

huan commented Aug 11, 2018

The best practice is to keep the date immutable as possible as we can.

If you want to change those values, you can do that by creating a new UrlLink.

Or could you provide a scenario that can prove we have to support mutation?

@windmemory
Copy link
Member Author

But even create a new UrlLink, the title, thumbnailurl and description is immutable.

I think either we support modifying the link or creating new UrlLink with full control.

I can not come up with use cases modifying the UrlLink, but create a customize link should be common scenario.

What do you think?

@huan
Copy link
Member

huan commented Aug 12, 2018

Please file another issue for this immutable discuss because this is a closed PR, and let's track it more clear.

@windmemory
Copy link
Member Author

Okay, issue created: #1541

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants