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

improve random strings #281

Merged
merged 11 commits into from
Jul 23, 2022
Merged

improve random strings #281

merged 11 commits into from
Jul 23, 2022

Conversation

JanMarvin
Copy link
Owner

This is a bit tricky and I'd be open to suggestions how to improve this or if we are fine with this. I needed it, to run a test and it works well. Therefore ... another suggestion must be better :)

This approach does not alter the seed when calling random_string(), even with strings of random sizes. I applied these in a row, but ran into a problem, where I actually wanted the seed to be altered when calling it in openxlsx2, therefore I've added a way for openxlsx2 to handle such cases.

Once random_string()is called:

  1. We backup the current seed and check for .openxlsx2.seed, this is the seed we want to use.
  2. If none is found, we create one ourself.
  3. Afterwards we use stringi to create a random string for us.
  4. We store the altered string as .openxlsx2.seed therefore once we call random_string() again, it will pick up this seed.
  5. We restore the previous global seed as if it was never altered.

@JanMarvin
Copy link
Owner Author

The onus is, we pollute the .GlobalEnv with our seed and if we want to set the seed for random_seed() we have to remove .openxlsx2.seed previously.

@JanMarvin
Copy link
Owner Author

I use this in #280

R/utils.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@jmbarbone jmbarbone left a comment

Choose a reason for hiding this comment

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

withr::with_seed() also checks seed type, which we may want to do as well: https://github.com/r-lib/withr/blob/main/R/seed.R

R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
@JanMarvin
Copy link
Owner Author

fixed the tests

@JanMarvin
Copy link
Owner Author

We could add a seed counter option. We set and increase in random_string() and use set.seed() with it. That would be the slimmest approach, but personally I'm fine with both. The current in this PR and the sketched idea as well. I see if I have time to add this later today, otherwise I'll merge with a to-do note. It's a minor function without much use at the moment. Unless someone wants to add styles or sparklines it isn't triggered at all.

@JanMarvin
Copy link
Owner Author

Scratch that last comment. Check complains therefore I'll change it.

Found the following assignments to the global environment:
File ‘openxlsx2/R/utils.R’:
  assign(".openxlsx2.seed", .Random.seed, globalenv())

@JanMarvin JanMarvin merged commit 9d46e56 into main Jul 23, 2022
@JanMarvin JanMarvin deleted the random_strings branch July 23, 2022 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants