Skip to content

refactor: introduce Source trait for random generator #1910

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 2 commits into
base: main
Choose a base branch
from

Conversation

peter-jerry-ye
Copy link
Collaborator

Closes #1589

Copy link

peter-jerry-ye-code-review bot commented Apr 9, 2025

Missing type constraint on `Source` implementation

Category
Correctness
Code Snippet
impl Source for @random_source.ChaCha8 with next(self : @random_source.ChaCha8) -> UInt64
Recommendation
Add a check to ensure source implementation can generate values in the expected range
Reasoning
The Source trait implementation should guarantee that generated numbers meet certain quality requirements. Without constraints, implementations could provide poor quality or biased random numbers.

Deprecated function still used in documentation example

Category
Maintainability
Code Snippet
test {
let r = @random.new()
...
}
Recommendation
Update the example in README.mbt.md to use the new constructor style:

let r = @random.Rand::new(@random.chacha8())

Reasoning
Documentation should reflect current best practices and avoid showing deprecated APIs to prevent new code from using outdated patterns.

Missing documentation for trait methods

Category
Maintainability
Code Snippet
pub(open) trait Source {
next(Self) -> UInt64
}
Recommendation
Add documentation for the next method explaining its contract:

pub(open) trait Source {
  ///| Generates the next 64-bit unsigned random integer
  next(Self) -> UInt64
}

Reasoning
Trait method documentation is crucial for implementers to understand the requirements and guarantees they need to provide.

@coveralls
Copy link
Collaborator

coveralls commented Apr 9, 2025

Pull Request Test Coverage Report for Build 6209

Details

  • 4 of 7 (57.14%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 91.52%

Changes Missing Coverage Covered Lines Changed/Added Lines %
random/random.mbt 4 7 57.14%
Totals Coverage Status
Change from base Build 6202: -0.05%
Covered Lines: 5634
Relevant Lines: 6156

💛 - Coveralls

@bobzhang bobzhang force-pushed the zihang/migrate-rand branch from 88a992d to f89b61d Compare April 13, 2025 06:28
@@ -24,7 +27,7 @@ impl Rand {
float(Self) -> Float
int(Self, limit~ : Int = ..) -> Int
int64(Self, limit~ : Int64 = ..) -> Int64
new(seed~ : Bytes = ..) -> Self //deprecated
new(&Source) -> Self
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

add a default value

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.

[Refactor API] Separate Rand and Source
2 participants