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

chore: Replace deprecated rand.Seed with dedicated datagen random generators #363

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

vitsalis
Copy link
Member

rand.Seed became deprecated with Go 1.20 (see notes). In this PR we add an additional argument to all datagen methods specifying the randomness generator they'll use. The tests across the codebase have been modified to reflect that.

@SebastianElvis
Copy link
Member

Is it possible to create a public Seed variable in datagen, initialise this seed using the init function, and reuse this seed everywhere? so that we can keep the original interfaces

@KonradStaniec
Copy link
Collaborator

Is it possible to create a public Seed variable in datagen, initialise this seed using the init function, and reuse this seed everywhere? so that we can keep the original interfaces

Tbh I think that this way is a bit better. Imo init function creates a bit not obvious control flow, and if someone is not aware that there is init called somewhre then it is really hard debug what is going on. On the other hand, passing parameter to function in quite obvious for everybody. And it is not like we have any commitment, to keeping the interfaces in current shape.

One thing, I am not sure about is, maybe it is better for our datagen functions to take Source interface as parameter, instead of random.Rand which is concrete implementation ? This is not a blocker, just discussion initiation :)

@vitsalis
Copy link
Member Author

Is it possible to create a public Seed variable in datagen, initialise this seed using the init function, and reuse this seed everywhere? so that we can keep the original interfaces

Tbh I think that this way is a bit better. Imo init function creates a bit not obvious control flow, and if someone is not aware that there is init called somewhre then it is really hard debug what is going on. On the other hand, passing parameter to function in quite obvious for everybody. And it is not like we have any commitment, to keeping the interfaces in current shape.

One thing, I am not sure about is, maybe it is better for our datagen functions to take Source interface as parameter, instead of random.Rand which is concrete implementation ? This is not a blocker, just discussion initiation :)

For me, the datagen methods should be like "Generate me this data and do it from this randomness engine". Whereas passing Source would be like "Generate me this data by creating a randomness engine with this source". I'm in favor of the first approach. Given that the ultimate thing the datagen methods depend on is the randomness engine, we can pass that to have more control over the engine (instead of just passing a source that the datagen methods are in control of). For example, we might want the random data to be generated from a seeded randomness engine that has already generated 3 integers. Further, the datagen methods won't bother themselves with creating a rand.Rand object which is unrelated to their functionality imo and would have to be repeated extensively across the methods.

@KonradStaniec
Copy link
Collaborator

KonradStaniec commented Apr 26, 2023

Whereas passing Source would be like "Generate me this data by creating a randomness engine with this source".

Hmm I would say that it rather like Generate me this data, this is your randomness source. There is no assumption by the caller that randomness engine will be created (although that would most probably happen in practice)

Although rest of a the answer makes sense, mostly the fact that indeed using interface may lead in practice to a bit of duplicated logic. Lets leave it like this then, and see how we will like it 👍

@vitsalis vitsalis merged commit 795d706 into dev Apr 26, 2023
@vitsalis vitsalis deleted the vitsalis/replace-rand-seed branch April 26, 2023 07:00
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