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

Enhancement: Configure random_api_migration fixer #692

Closed

Conversation

localheinz
Copy link
Member

What is the reason for this PR?

  • configures the random_api_migration fixer
  • runs make cs

Author's checklist

Summary of changes

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@localheinz localheinz self-assigned this Sep 2, 2023
@localheinz localheinz marked this pull request as draft September 2, 2023 10:15
@bram-pkg
Copy link
Member

bram-pkg commented Sep 3, 2023

Wouldn't this break mt_seed?

@localheinz
Copy link
Member Author

@bram-pkg

I think the entire randomization needs a bit of rework!

Given that randomization is infrastructure (similar to a clock), I think it makes sense to abstract it away. Then we can change the internal implementation to whatever makes the most sense, and eventually migrate away towards https://www.php.net/manual/en/class.random-randomizer.php.

What do you think?

@bram-pkg
Copy link
Member

bram-pkg commented Sep 3, 2023

@bram-pkg

I think the entire randomization needs a bit of rework!

Given that randomization is infrastructure (similar to a clock), I think it makes sense to abstract it away. Then we can change the internal implementation to whatever makes the most sense, and eventually migrate away towards https://www.php.net/manual/en/class.random-randomizer.php.

What do you think?

Sounds perfect to bind an instance of that class to our container, so we can resolve the randomiser from anywhere.

@localheinz localheinz closed this Sep 6, 2023
@localheinz localheinz deleted the feature/random-api-migration branch September 6, 2023 10:36
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