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.

Sign up for GitHub

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

Jump to bottom

Fixed CreateTodo method #1441

Closed
wants to merge 1 commit into from
Closed

Fixed CreateTodo method #1441

wants to merge 1 commit into from

Conversation

Surfoo
Copy link

@Surfoo Surfoo commented Jan 19, 2021

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.398% when pulling 08643ee on Surfoo:master into 997da42 on 99designs:master.

milosgajdos
milosgajdos reviewed Mar 1, 2021
View reviewed changes
docs/content/getting-started.md
@@ -114,9 +114,10 @@ we create the graph.

```go
func (r *mutationResolver) CreateTodo(ctx context.Context, input model.NewTodo) (*model.Todo, error) {
id, _ := rand.Int(rand.Reader, big.NewInt(100000000))
Copy link

Choose a reason for hiding this comment

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

never leave an error unhandled

@MichaelJCompton
Copy link
Contributor

Hi, thanks for submitting the PR.

I'm not clear on why this is a 'fix' for the method. Please supply some context.

@Surfoo
Copy link
Author

Surfoo commented Mar 15, 2021

Hello,

Did you really do the whole tutorial?
It's rand.Int from the crypto pkg in the generated code, not from math pkg.

rand.Int on https://golang.org/pkg/crypto/rand/#Int needs rand io.Reader, max *big.Int in input and returns a big.Int and an error in output. The actual code in the tutorial doesn't compile.
I assumed we don't care about the error, it's a tutorial :)

And there are still other mistakes inside..

@Surfoo
Copy link
Author

Surfoo commented Mar 15, 2021

#1274

@MichaelJCompton
Copy link
Contributor

MichaelJCompton commented Mar 15, 2021 •
edited

Hi, I think the answer on the issue you linked is what you are looking for:

#1274 (comment)

It doesn't say in the tutorial to use crypto, but I'll update the text to make it mentions math.rand.

I'll close this issue and please report the other errors as new issues so I can track them, thanks.

@MichaelJCompton MichaelJCompton mentioned this pull request Mar 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milosgajdos milosgajdos milosgajdos left review comments

Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Surfoo @coveralls @MichaelJCompton @milosgajdos