-
Notifications
You must be signed in to change notification settings - Fork 25
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
Make RegexGenerator
generate unlimited regexes even when enforce_uniqueness=True
#753
Conversation
RegexGenerator
generate unlimited regexes when enforce_unique=True
RegexGenerator
generate unlimited regexes when enforce_unique=True
RegexGenerator
generate unlimited regexes even when enforce_uniqueness=True
d7f7302
to
a66edd0
Compare
a66edd0
to
f1cc267
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one question about messaging.
self.reset_randomization() | ||
remaining = self.generator_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, why do we warn the user and request them to reset randomization if we do it here for them anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just let one question
rdt/transformers/text.py
Outdated
|
||
while len(reverse_transformed) < sample_size: | ||
remaining_samples = sample_size - len(reverse_transformed) | ||
if dtype == int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the while
boucle for the int
case? It seems likely to resolve in one iteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I missed that.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #753 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 1960 1975 +15
=========================================
+ Hits 1960 1975 +15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
CU-86ayz97gr, Resolve #749