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

fix #2485 for some types requiring a scalar #2508

Merged
merged 1 commit into from
Jan 16, 2023

Conversation

mstephano
Copy link
Contributor

@mstephano mstephano commented Jan 14, 2023

Following PR # 2486, some types require a scalar to work properly. This PR fixes some of them except float32 and uint64. The reasons are as follow:

  • float32: the precision changed when marshalling/unmarshalling from float64 back to float32. As an example, value of 5.76 comes out as 5.760000228881836. Anyone knows how to fix this? You can see this branch for an example, or precisely this commit.
  • uint64: it is bigger than int64

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

Coverage Status

Coverage: 75.54% (-0.04%) from 75.579% when pulling 2c266a3 on mstephano:fix/unusual-basic into 11c3a4d on 99designs:master.

@@ -257,6 +257,30 @@ func (t *TypeReference) IsUnderlyingBasic() bool {
return isUnderlyingBasic
}

func (t *TypeReference) IsUnusualBasic() bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named these as Unusual but a suggestion for a better name is welcomed.

@StevenACoffman StevenACoffman merged commit ec3b471 into 99designs:master Jan 16, 2023
@mstephano mstephano deleted the fix/unusual-basic branch January 16, 2023 12:43
@mojtabacazi
Copy link

This commit actually broke our app, it generates invalid code when trying to unmarshal typed scalars. It evens breaks time.Duration types to name a few. Here is an example:

func (ec *executionContext) unmarshalNInt642timeᚐDuration(ctx context.Context, v interface{}) (time.Duration, error) {
	res, err := graphql.UnmarshalInt64(v)
	return res, graphql.ErrorOnPath(ctx, err)
}

which causes this error:

cannot use res (variable of type int64) as type time.Duration in return statement

Here is another example with a pointer type

func (ec *executionContext) unmarshalOBoolean2DummyFilter(ctx context.Context, v interface{}) (*dummy.DummyFilter, error) {
	if v == nil {
		return nil, nil
	}
	res, err := graphql.UnmarshalBoolean(v)
	return &res, graphql.ErrorOnPath(ctx, err)
}

@StevenACoffman
Copy link
Collaborator

@mojtabacazi Did #2528 fix your issue?

@StevenACoffman
Copy link
Collaborator

also related: #2486

@mojtabacazi
Copy link

mojtabacazi commented Mar 16, 2023

@mojtabacazi Did #2528 fix your issue?

Nope, still an issue in v0.17.26

StevenACoffman added a commit that referenced this pull request Mar 20, 2023
Signed-off-by: Steve Coffman <steve@khanacademy.org>
StevenACoffman added a commit that referenced this pull request Mar 20, 2023
Signed-off-by: Steve Coffman <steve@khanacademy.org>
@StevenACoffman
Copy link
Collaborator

@mojtabacazi I reverted this, so please see if v0.17.27 resolves your issue!

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