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

RegexGenerator gives a confusing message: # of possibilities are shown as an imaginary number #754

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

R-Palazzo
Copy link
Contributor

CU-86ayz967f
Resolve #748

I didn't use f-string formatting because it was raising this lint warning for info message:
rdt/transformers/text.py:166:12: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)

Also you can see with this screenshot of the code of step to reproduce that we now print real values.
Screenshot 2024-01-17 at 10 07 59

@R-Palazzo R-Palazzo requested a review from a team as a code owner January 17, 2024 11:01
@sdv-team
Copy link
Contributor

@R-Palazzo R-Palazzo removed the request for review from a team January 17, 2024 11:01
Comment on lines +166 to 170
LOGGER.info(
"The data has %s rows but the regex for '%s' can only create %s unique values."
" Some values in '%s' may be repeated.",
sample_size, self.get_input_column(), self.generator_size, self.get_input_column()
)
Copy link
Member

Choose a reason for hiding this comment

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

We could do:

message = (
    f"..."
    f"..."
)

But it is actually fine to have it as it is right now; The reason why, f-string gets 'compiled' always, meanwhile this way of string formatting gets generated only when called, which means that it will be generated only when logger is enabled.

Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@fealho fealho left a comment

Choose a reason for hiding this comment

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

LGTM

@R-Palazzo R-Palazzo merged commit 9f37fb4 into main Jan 17, 2024
90 checks passed
@R-Palazzo R-Palazzo deleted the issue-748-regexgenerator-message branch January 17, 2024 16:13
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.

RegexGenerator gives a confusing message: # of possibilities are shown as an imaginary number
4 participants