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

Add length option for random string provider #31

Merged

Conversation

bhagenbourger
Copy link
Contributor

#18

@bhagenbourger bhagenbourger marked this pull request as draft March 8, 2024 16:49
@bhagenbourger
Copy link
Contributor Author

I added the "length" option that accepts constant and range.
Range must be specified like that : "3..20"
Constant must be specified like that : "8"

I'm not completely satisfied by this solution because the constant must be between double quotes. If an integer is specified the function column["length"].as_str() returns None.

@vianneybacoup do you know a better solution to be able to specify an integer or a string for the constant length?

Thank you for your help.

@vianneybacoup
Copy link
Collaborator

Thanks for the range idea @bhagenbourger, it would be a great fit for the random integer min/max as well !

An easy fix would be to cast the integer to str before the .map(), with a match to handle both types. I am not a fan of this solution but it's still better than having to put the quotes in the config file.
Having a dedicated logic for parameters (IntegerParam, RangeParam, DateParam, etc...) to avoid duplicating the logic would be a great enhancement and may be a clean fix for this "issue". But that's an extra feature/refactoring, I'll might do that in the next weeks.

Also the quotes for "5..15" in the yaml file are not necessary :)

@bhagenbourger bhagenbourger force-pushed the feature/length_for_string branch 6 times, most recently from fa62270 to e8f8c8a Compare March 10, 2024 14:07
@bhagenbourger bhagenbourger force-pushed the feature/length_for_string branch from e8f8c8a to c2eee67 Compare March 10, 2024 14:09
@bhagenbourger
Copy link
Contributor Author

bhagenbourger commented Mar 10, 2024

Thank you @vianneybacoup for your help, with pattern matching I was able to solve my issue. Length option can now be specified into the yaml without double quotes.
Agree with you about "Having a dedicated logic for parameters", I let this point for a specific issue.
I also modified tests/tests.rs to manage unix OS. I was able to test only on macos, I hope I didn't break anything on Windows.

@bhagenbourger bhagenbourger marked this pull request as ready for review March 10, 2024 14:27
Copy link
Collaborator

@vianneybacoup vianneybacoup left a comment

Choose a reason for hiding this comment

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

Thanks for this PR as well @bhagenbourger :)

@vianneybacoup vianneybacoup merged commit a3dc5f3 into soma-smart:main Mar 10, 2024
3 checks passed
vianneybacoup added a commit that referenced this pull request Mar 10, 2024
Add length option for random string provider

Co-authored-by: bhagenbourger <[email protected]>
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