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

replace math/rand #6674

Closed
wants to merge 0 commits into from
Closed

replace math/rand #6674

wants to merge 0 commits into from

Conversation

kushal9897
Copy link

@antoninbas
Copy link
Contributor

Please do not open a different PR for the same task in the future.
We lose the review history for no reason.

This PR replaces #6659

@antoninbas
Copy link
Contributor

@kushal9897 I do not think you took my advice from #6659 (review) in consideration. This should not be treated as a "search-and-replace" operation. You need to understand the changes introduced in math/rand/v2 by reading https://go.dev/blog/randv2 carefully, and in particular the Fixing the rest in math/rand/v2 section. Some issues are pretty obvious as the code no longer builds correctly with your changes. I recommend running make golangci locally to discover the most basic issues. Note that it may not be enough - it is possible that updating to math/rand/v2 will enable some improvements that can only be identified by reviewing usages of the math/rand package in the code base (cannot be identified through build failures). This is why reading that blog post carefully is important.

@kushal9897
Copy link
Author

Thank you for your feedback. I apologize for not fully considering your advice from #6659. You're right—I approached the changes more as a 'search-and-replace' operation without fully understanding the implications of the updates to math/rand/v2. I will carefully read the blog post, especially the 'Fixing the rest in math/rand/v2' section, and review the usage of the package in the codebase. I'll also run make golang locally to identify any further issues. I appreciate your guidance and will work on addressing this properly. Thank you again for your patience.

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