Skip to content

Conversation

@kkdai
Copy link
Member

@kkdai kkdai commented Apr 17, 2020

Copy link
Contributor

@clsung clsung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func (m *TextMessage) AddEmoji(emojis *Emoji) SendingMessage {

emojis -> emoji

func (m *TextMessage) AddEmoji(emoji *Emoji) SendingMessage {

@kkdai
Copy link
Member Author

kkdai commented Apr 21, 2020

@clsung update by your suggestion, thank you

@codecov-io
Copy link

codecov-io commented Apr 21, 2020

Codecov Report

Merging #202 into master will increase coverage by 0.66%.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   75.95%   76.61%   +0.66%     
==========================================
  Files          28       30       +2     
  Lines        2200     2254      +54     
==========================================
+ Hits         1671     1727      +56     
+ Misses        432      429       -3     
- Partials       97       98       +1     
Impacted Files Coverage Δ
linebot/message.go 77.56% <23.80%> (+0.29%) ⬆️
linebot/emoji.go 100.00% <100.00%> (ø)
linebot/event.go 88.02% <0.00%> (ø)
linebot/client.go 72.09% <0.00%> (ø)
linebot/progress.go 100.00% <0.00%> (ø)
linebot/response.go 70.52% <0.00%> (+0.21%) ⬆️
linebot/flex_unmarshal.go 68.85% <0.00%> (+1.63%) ⬆️
linebot/flex.go 47.61% <0.00%> (+4.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9b717e...aae3847. Read the comment docs.

return m
}

// AddEmoji method of TextMessage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe need to add this method to SendingMessage interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasa9084 -san,
The emoji value only works for TextMessage, and not working for ImageMessage or other message types.

If I add it as an interface for XendMessage, I might do nothing if message type is not TextMessage.

How do you think?

e.g.

// AddEmoji method of ImageMessage
func (m *ImageMessage) AddEmoji(emoji *Emoji) SendingMessage {
	return m
}

Copy link
Member

@nasa9084 nasa9084 Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm newer person for this project so I'm not sure though, I think the method is considered to be used as method chain like m.AddEmoji(emoji1).AddEmoji(emoji2).

I can see:

  1. AddEmoji is returning SendingMessage interface
  2. the field emojis is private field
  3. NewTextMessage does not have argument to set emojis

so users cannot set multiple emojis without type assertion, if SendingMessage does not have AddEmoji method. I think it is hard to use if we need to do like m.AddEmoji(emoji1).(*TextMessage).AddEmoji(emoji2)

I think we have some options:

  • Add AddEmoji method to SendingMessage interface
  • Change not to return SendingMessage for all methods, means remove SendingMessage interface
  • Change AddEmoji method to AddEmojis to be able to pass []*Emoji
  • Change the NewTextMessage to be able to pass emojis
  • ... or maybe other choice

HDYT?

cc/ @oklahomer @k2wanko

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasa9084 @oklahomer Any conclusion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasa9084 Could you check again? I have fixed your question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to be late reply, please wait for a while....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I checked and LGTM

@nasa9084 nasa9084 merged commit 2bf17fd into line:master May 11, 2020
@nasa9084
Copy link
Member

sorry to be late merging

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.

Support LINE emoji in messages

4 participants