Skip to content

fix: modify argment variables - #17

Merged
miseyu merged 3 commits into
mainfrom
fix/modify-Argument-Values
Jun 5, 2023
Merged

fix: modify argment variables#17
miseyu merged 3 commits into
mainfrom
fix/modify-Argument-Values

Conversation

@miseyu

@miseyu miseyu commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

Issue

  • Changed to consider nil in variables

TL; DR

  • TBA

Task summary / Change details

  • TBA

@miseyu miseyu self-assigned this Jun 5, 2023
@miseyu
miseyu requested a review from ikawaha June 5, 2023 04:04
@miseyu miseyu added the Type: Bug Something isn't working label Jun 5, 2023

@ikawaha ikawaha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ちょっとよく理解できてないので、コメントだけ失礼します。

修正の意図としては 「要素が null の時には省略せず、null を返してほしい」 という事になるでしょうか。

元のコードで判定に使われていた isNullish 関数はコード的には、値が

  • nil
  • 型付きの nil
  • 数値の不正な値
  • undefined

のときに true を返すとしているようなので、単純に null である場合以外も扱っているような感じがします。ここの isNullish を完全に外してしまっていいのが不安になりました(←理解できてない)。

( '-`).oO( isNullish は undefined をどう扱ってるんですかね(string の条件のところ?)

https://github.com/tailor-inc/graphql/blob/bb32401adc56c318dd10411f5fe184ebc345f134/values.go#L299-L323

@miseyu

miseyu commented Jun 5, 2023

Copy link
Copy Markdown
Contributor Author

@ikawaha
すみません、もう一度ご確認いただけますでしょうか??

Comment thread values.go Outdated
if !isNullish(value) {
obj[name] = value
if isUndefined {
if !isNullish(value) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fieldASTs は filed にある場合です。graphqlの。この場合はundefinedはないのですが、念のため同じ実装にしています。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fieldASTsは戻り値のfieldになります。

Comment thread values.go
)
if tmpValue, ok := argASTMap[argDef.PrivateName]; ok {
value = tmpValue.Value
} else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

argASTMap にフィールドがあるかないかでundefinedを考慮しています。
argASTMapには、実際の引数の情報が入ってきます。

@ikawaha ikawaha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ありがとうございます!やっと理解できました (あつかいがむつかしい・・・
このままで問題ないと思いますが、値を設定するところの条件をすこしコンパクトに出来そうだったのでコメントしました。ご確認ください。

Comment thread values.go Outdated
Comment thread values.go Outdated
@miseyu

miseyu commented Jun 5, 2023

Copy link
Copy Markdown
Contributor Author

@ikawaha
ありがとうございます。修正して、こちらでマージします。

@miseyu
miseyu merged commit 1feac02 into main Jun 5, 2023
@miseyu
miseyu deleted the fix/modify-Argument-Values branch June 5, 2023 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants