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

Optionalフィールドの設定 #29

Closed
wants to merge 2 commits into from
Closed

Conversation

logica0419
Copy link
Contributor

いくつかのフィールドは「0値」と「無い」を明示的に分ける必要を感じたので、optionalを設定した
それに伴って「HighestScore」は「BestScore」になった(高い = 良いわけではないので)

@logica0419 logica0419 requested a review from oribe1115 June 17, 2022 13:37
@oribe1115
Copy link
Contributor

Optional、genericsの使い所じゃないですか?:eyes:
こんな感じで

type optionalType interface {
	string | float64
}

type Optional[T optionalType] struct {
	Value T
	Valid bool
}

jsonのパースにencoding/jsonではなくgithub.com/json-iterator/goを使ってるのは速度が速いのが使いたかったってことですかね?

@logica0419
Copy link
Contributor Author

json-iteratorに関しては、早いのもありますが、traQから引っ張ってきてるのでこれになってます
別にそうじゃなくても問題ないので、標準パッケージでいっか…と今思っているところです

@logica0419
Copy link
Contributor Author

logica0419 commented Jun 18, 2022

generics型のメソッドの中で、型パラメーター使ってconditionalな記述するの、ベストプラクティスではない気がしていて分けています
ここはgenericsではなく、インターフェースの実装という形で表現されるのが適切かな〜と思ってるんですがそこのところどう考えてますか?

@logica0419
Copy link
Contributor Author

logica0419 commented Jun 18, 2022

見比べて貰えばわかるんですが、Marshal系が一部処理違うんですよね…

@oribe1115
Copy link
Contributor

oribe1115 commented Jun 18, 2022

Optionlな型のメインの値の型を型パラメーターで渡すこと自体には違和感は感じてないです
JavaのOptional型とかそうですし

Marshal系の処理は型スイッチで分岐すればいいのでは?と思ってたけれど、型パラメーターまわりの型スイッチはまだ検討中なんですね
golang/go#49206
現状でも一応こういうことはできるみたいです

switch v := ((any)(value)).(type) {
	case string:
		fmt.Print(v) // string
	}

私としては、Optional[T]の方が読みやすい&対象の型の拡張もしやすいとおもっています

@oribe1115
Copy link
Contributor

書いてみたけれどそもそも型スイッチいらなさそう?
#40

@logica0419
Copy link
Contributor Author

#40 にoptionalの実装を変えるのでclose

@logica0419 logica0419 closed this Jun 18, 2022
@logica0419 logica0419 deleted the fix/schema-optional branch June 18, 2022 05:31
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