Skip to content

Remove random.Seed() call from robot-name's approach file #2853

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HerrSubset
Copy link

From the Go documentation on random.Seed():

Deprecated: As of Go 1.20 there is no reason to call Seed with a random value. Programs that call Seed with a known value to get a specific sequence of results should use New(NewSource(seed)) to obtain a local random generator.

@HerrSubset
Copy link
Author

Just created a forum post for this issue as well.

@siebenschlaefer
Copy link
Contributor

I'm not involved in the Go track in any way, but I just noticed:
You might want to make that same change in .approaches/shuffle/content.md, too, to keep the files in sync.

From the Go documentation on random.Seed():

Deprecated: As of Go 1.20 there is no reason to call Seed with a random value.
Programs that call Seed with a known value to get a specific sequence of results
should use New(NewSource(seed)) to obtain a local random generator.

The time import could then also be removed, because it was only used
when setting the seed.
@HerrSubset
Copy link
Author

Thanks for checking that @siebenschlaefer, I updated the PR.

I also tested both examples and saw that the time import was no longer needed, so removed that too from both files. Both examples now compile and pass the tests!

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

Looks good. The sample solution didn't seed a random number generator.

Can you confirm these approaches actually pass the tests if ran? If yes, I don't see a reason why this cannot be merged...

@SleeplessByte
Copy link
Member

cc @exercism/go

@HerrSubset
Copy link
Author

Can you confirm these approaches actually pass the tests if ran? If yes, I don't see a reason why this cannot be merged...

Yes, tested that last week after siebenschlaefer's comment :)

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.

3 participants