Skip to content

Conversation

@sugyan
Copy link
Contributor

@sugyan sugyan commented Feb 12, 2019

Fix #124.

this is breaking change

For enable to pass 0 as value, changed the type of flex field from int to FlexValueType (equals to *int), so developers need to change values to FlexValueType. So I added a helper function FlexInt() to create FlexValueType value.

linebot/flex.go Outdated
type FlexValueType *int

// FlexValue is helper function for using FlexValueType values
func FlexValue(v int) FlexValueType {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the function name better be FlexInt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I renamed it.

Copy link
Contributor

@k2wanko k2wanko left a comment

Choose a reason for hiding this comment

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

I gave a little comment. 👀

@sugyan
Copy link
Contributor Author

sugyan commented Feb 12, 2019

Fixed tests 🙇

@linxGnu
Copy link
Contributor

linxGnu commented Feb 14, 2019

@sugyan Could you please describe why we should have this?

Referencing type FlexValueType *int could cause data race in user's application. It's better to make it immutable like before.

My suggestion is just letting:

type FlexValueType = int // alias type

And no need:

func FlexInt(...)

@k2wanko
Copy link
Contributor

k2wanko commented Feb 14, 2019

@linxGnu Good suggestion.
In adopting this method, it seems necessary to end support of less than 1.9.

https://github.com/line/line-bot-sdk-go/blob/bc678e0b74717c1daf0836e13bbed9dd3e6988b8/README.md#requirements

@linxGnu
Copy link
Contributor

linxGnu commented Feb 15, 2019

@k2wanko I think so. I believe user has upgrade to go 1.9 for huge improvement at GC and runtime. So I do not think we need to support go1.7 and 1.8.

@sugyan
Copy link
Contributor Author

sugyan commented Feb 15, 2019

@linxGnu Thanks for your reviewing and comments.
I updated my branch, but your proposal was not such a change?
If you know a better solution for #124, I'd be happy if you give another pull-request.

@linxGnu
Copy link
Contributor

linxGnu commented Feb 16, 2019

@sugyan I think you may misunderstand my comment.

type FlexValueType *int

You should not do that. Because you are making reference to an int outside. I think in our case, FlexValueType should be immutable. So it should be like this:

type FlexValueType = int

Yes, it does not change anything!
But we still archive the goal of making code/type clearer.

@sugyan
Copy link
Contributor Author

sugyan commented Feb 19, 2019

I updated my branch again. How about this?

linebot/flex.go Outdated
type FlexValueType = int // alias type

// FlexInt is a helper function for using *FlexValueType values
func FlexInt(v int) *FlexValueType {
Copy link
Contributor

Choose a reason for hiding this comment

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

FlexInt is not necessary when using type alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think *FlexValueType is not necessary. I wonder why we have to embedded the * (reference) instead of value 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you read #124 ?
flex is an optional field and tagged as omitempty, but it should accept the value 0 as not the default value.
So I thought that we have to embed the * (reference) instead of value to omit this field only if the reference is null.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 @sugyan Sound reasonable to me. Thank you very much for explanation.

@k2wanko k2wanko dismissed their stale review February 19, 2019 04:07

I'm sorry. It is necessary for assembling the structure.

@sugyan sugyan requested a review from linxGnu February 19, 2019 04:20
Copy link
Contributor

@linxGnu linxGnu left a comment

Choose a reason for hiding this comment

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

LGTM

@mokejp
Copy link
Contributor

mokejp commented Feb 20, 2019

It looks good in this case.
However, I am concerned that similar types will increase if optional fields increase in the future.

I think that using *int and adding func Int(v int) *int helper function is the simple and versatile way.

@sugyan
Copy link
Contributor Author

sugyan commented Feb 20, 2019

Thanks for your comment.
I removed alias type and renamed the helper function to IntPtr.

Copy link
Contributor

@mokejp mokejp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@sugyan sugyan merged commit 50b8ef0 into line:master Feb 20, 2019
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