Skip to content

Conversation

@clsung
Copy link
Contributor

@clsung clsung commented Feb 4, 2020

- implments line#190
TODO:
- narrowcast progress API
@codecov-io
Copy link

codecov-io commented Feb 4, 2020

Codecov Report

Merging #191 into master will increase coverage by 0.69%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   75.59%   76.29%   +0.69%     
==========================================
  Files          25       27       +2     
  Lines        2000     2143     +143     
==========================================
+ Hits         1512     1635     +123     
- Misses        394      411      +17     
- Partials       94       97       +3
Impacted Files Coverage Δ
linebot/response.go 70.3% <ø> (+0.43%) ⬆️
linebot/client.go 72.09% <ø> (ø) ⬆️
linebot/recipient.go 81.48% <81.48%> (ø)
linebot/send_message.go 90.44% <81.57%> (-2.84%) ⬇️
linebot/demographic_filter.go 87.75% <87.75%> (ø)
linebot/insight.go 100% <0%> (ø) ⬆️
linebot/event.go 88.02% <0%> (+0.14%) ⬆️

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 18ff60a...bb527f8. Read the comment docs.

}

// AudienceFilter type
type AudienceFilter struct {
Copy link
Member

Choose a reason for hiding this comment

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

The Audience object is not an demographic filter object, it is a recipient object.
Currently recipient object and demographic filter object have (looks) same operator object but they are not exactly same, they can be altered for each.
So I think it is better to split operator struct to two, 1. recipient operator object 2. demographic filter operator object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, will update later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I've renamed filter to selector to remedy the semantics. Thanks.

@nasa9084
Copy link
Member

I think interfaces for audience and other filters should be separately defined, such as:

type Recipient interface {
   // method
}

type DemographicFilter interface {
  // method
}

and Audience implements Recipient and other demographic filter objects implements DemographicFilter

sorry my lack of explanation

@oklahomer
Copy link
Contributor

Since recipient and demographic filter are entirely different objects, can we treat them as such?
The current implementation shares the same interface such as Selector, but I think it should be separated as they have no common ancestry.
And hence, the Operator itself will be separated for both as a result since this is not meant to be shared.

@nasa9084 nasa9084 requested a review from oklahomer February 21, 2020 04:42
@clsung
Copy link
Contributor Author

clsung commented Feb 22, 2020

Since recipient and demographic filter are entirely different objects, can we treat them as such?
The current implementation shares the same interface such as Selector, but I think it should be separated as they have no common ancestry.
And hence, the Operator itself will be separated for both as a result since this is not meant to be shared.

Hi,

I'm not the one who designs these filters, not sure if I missed the design proposal. Here's my initiatives:

  1. Interface in Go provide a way to specify the behavior of an object: if something can do this, then it can be used here. On the other hand, a type can implement multiple interfaces. I think if we check the JSON output of recipient object and demographic filter, we can't literally distinguish between recipient or and demographic filter or'. Same thing applies to and and not.

  2. After look again into Operator/Selector code, I think I should name it as Select() method and Selector interface in order to comply with Go coding guideline.

How do you think?

@oklahomer
Copy link
Contributor

My apologies for the interruption and confusion.

I thought the point was to separate Operator to simplify the interface/implementation: One operator for Recipient and another for Demographic filter.

The Recipient document says the operator deals with Recipient objects while Demographic filter's operator deals with Demographic filter objects. If we simply follow those definitions, we would have two different operators: RecipientOperator and DemographicFilterOperator. In this way, the interface and its implementation of the current Selector interface would also be distinguished accordingly and the overall design could be more simplified and type-safe, I suppose.

@clsung
Copy link
Contributor Author

clsung commented Feb 23, 2020

My apologies for the interruption and confusion.

I thought the point was to separate Operator to simplify the interface/implementation: One operator for Recipient and another for Demographic filter.

The Recipient document says the operator deals with Recipient objects while Demographic filter's operator deals with Demographic filter objects. If we simply follow those definitions, we would have two different operators: RecipientOperator and DemographicFilterOperator. In this way, the interface and its implementation of the current Selector interface would also be distinguished accordingly and the overall design could be more simplified and type-safe, I suppose.

Ok, I think it's fine to separate them. Updated.

Copy link
Contributor

@oklahomer oklahomer left a comment

Choose a reason for hiding this comment

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

Thank you for those changes. LGTM 👍

@nasa9084
Copy link
Member

LGTM,

@nasa9084 nasa9084 merged commit ffbe5cb into line:master Feb 25, 2020
clsung added a commit to clsung/line-bot-sdk-go that referenced this pull request Feb 26, 2020
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.

4 participants