Skip to content

[SPARK-47854][PYTHON][CONNECT] Avoid shadowing python built-ins in python function variable naming#49326

Closed
skanderboudawara wants to merge 9 commits intoapache:masterfrom
skanderboudawara:dev_sbu_SPARK-47854
Closed

[SPARK-47854][PYTHON][CONNECT] Avoid shadowing python built-ins in python function variable naming#49326
skanderboudawara wants to merge 9 commits intoapache:masterfrom
skanderboudawara:dev_sbu_SPARK-47854

Conversation

@skanderboudawara
Copy link
Copy Markdown

What changes were proposed in this pull request?

Change all the functions parameters shadowing python built-in to a distinguished ones.

Why are the changes needed?

As mentionned in the Jira ticket

# breaks:
foo(str="x", bar="y")

# okay:
foo("x", bar="y")

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

Github actions

Was this patch authored or co-authored using generative AI tooling?

No



def expr(str: str) -> Column:
def expr(expression: str) -> Column:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is actually a breaking change if users use this API via kwargs ..

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Shall I add to keep both versions?

@overload
def expr(str: str) -> Column: …

Or else I create a decorator for future releases like so

from functools import wraps

def deprecated_param(old_param: str, new_param: str):
    def decorator(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            if old_param in kwargs:
                import warnings
                warnings.warn(f"'{old_param}' is deprecated, use '{new_param}' instead", DeprecationWarning)
                kwargs[new_param] = kwargs.pop(old_param)
            return func(*args, **kwargs)
        return wrapper
    return decorator

@deprecated_param("str", "expression")
def expr(expression: str): ...

what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can do but i feel like deprecating the parameter is too much too. Shadowing the builtin names are not nice but it won't affect end users anyway

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hey,

I think my decorator needs another update. Since we can deprecate more than one parameter, I’m wondering if it makes sense to use a dictionary or maybe something else.

On the consistency side, all the new functions since version 3.3 use “col” instead of “str” for parameters. It feels like harmonizing everything would be a good move, but as you mentioned, it could be a breaking change for folks using keyword arguments.

What do you think?
Should we wait for more opinions on this topic ?

@skanderboudawara skanderboudawara changed the title [WIP][SPARK-47854][PYTHON][CONNECT] Avoid shadowing python built-ins in python function variable naming [SPARK-47854][PYTHON][CONNECT] Avoid shadowing python built-ins in python function variable naming Jan 3, 2025
@github-actions
Copy link
Copy Markdown

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Apr 16, 2025
@github-actions github-actions bot closed this Apr 17, 2025
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.

2 participants