-
Notifications
You must be signed in to change notification settings - Fork 172
feat: Add prefix argument to generate_temporary_column_name
#3147
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
Conversation
for more information, see https://pre-commit.ci
prefix argument to generate_temporary_column_name
| # Prepend `'nw'` to ensure it always starts with a character | ||
| # https://github.com/narwhals-dev/narwhals/issues/2510 | ||
| token = f"nw{token_hex(n_bytes - 1)}" | ||
| token = f"{prefix}{token_hex(n_bytes - 1)}" |
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.
Should we ensure that prefix is not an empty string?
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.
naah
MarcoGorelli
left a comment
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 @FBruzzesi !
| # Prepend `'nw'` to ensure it always starts with a character | ||
| # https://github.com/narwhals-dev/narwhals/issues/2510 | ||
| token = f"nw{token_hex(n_bytes - 1)}" | ||
| token = f"{prefix}{token_hex(n_bytes - 1)}" |
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.
naah
Still need to do `temp.column_names` as well That one is quite different to #3147, but was needed in this PR

What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below
Added explicit prefix only on lazy backends operations since these are the ones that a user might be able to investigate in various way. Not really applicable in the eager case