Skip to content

feat: random integer iterators and streams #84

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

Conversation

studiosi
Copy link

No description provided.

@studiosi studiosi changed the title feat: random integers wip: random integers Mar 11, 2025
@studiosi studiosi marked this pull request as draft March 11, 2025 12:55
@studiosi studiosi marked this pull request as ready for review March 11, 2025 14:34
@studiosi studiosi changed the title wip: random integers feat: random integer iterators and streams Mar 11, 2025
@coveralls
Copy link

coveralls commented Mar 12, 2025

Pull Request Test Coverage Report for Build 13788547605

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 24 of 24 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 13062161073: 0.0%
Covered Lines: 1482
Relevant Lines: 1482

💛 - Coveralls

@Smoren
Copy link
Owner

Smoren commented Mar 23, 2025

Hi @studiosi! Thank you for PR!
I see that you used a third-party library to implement new functionality. The thing is, I would like itertools to remain a dependency-free library, so I would like to ask you to rework this PR so that new features do not use a third-party library.

I would also like to ask you to implement tests using data providers, similar to the tests written earlier.

Thanks!

@studiosi
Copy link
Author

I don't understand the rationale of including a statistical self-coded random number generator in this library. It is a complex change that would introduce a lot of possible bugs and maintenance costs. If the choice is math.Random this library would be useless for most serious purposes, can you clarify what's the expected outcome?

@Smoren
Copy link
Owner

Smoren commented Apr 23, 2025

Hi @studiosi!

Have you ever encountered issues in a project due to transitive dependencies? For example, when a library requires one version of another library as a dependency, but your project needs a different version of it?
This is quite a common problem, so I want to keep itertools-ts free from any dependencies.

Also, I'd like to add that itertools-ts is a general-purpose library, and using the standard Math.random() will be perfectly fine for it. Those who need more powerful tools for working with random numbers can use the random library directly.

@studiosi
Copy link
Author

Yes, I know why not having dependencies is desirable. I also think that implementing a statistically sound random number generator should not be something that this library should do.

@Smoren
Copy link
Owner

Smoren commented Apr 28, 2025

@studiosi I think that using Math.random() is a better decision than adding transitional dependencies.

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