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 keyword arg guidelines to contrib doc #775

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jul 5, 2023

In response to some discussion around **kwargs, we agreed that I should take a first crack at these guidelines.

I tried to provide some context without lengthy exposition, and I've included a link to the PEP which introduced the * marker.
One thing I debated, but decided against, was any note about "new functions" (vs in general) and the theme that these rules are broken not-infrequently for backwards compatibility. I think that's a given in any codebase with this level of history, so I opted to just make the blanket statements and trust that the compatibility concerns are implicitly clear.

Backwards compatibility around **kwargs capturing

I avoided explaining in the text of the contrib doc, but, to illuminate the mention of "a class of backwards compatibility concerns", here's a bit of detail.

There are cases in which adding keyword arguments is not incompatible or not majorly so. This is completely compatible:

# v1
def foo(**kwargs):
    return {k: v for k, v in kwargs.items() if v is not None}

# v2
def foo(bar: str | None = None, **kwargs):
    data = kwargs.copy()
    if bar is not None:
        data["bar"] = bar
    return {k: v for k, v in data.items() if v is not None}

But there are simple cases in which it becomes nasty quickly. Here's a trivial-if-silly example:

def foo(a: int | None = None, b: int | None = None, **kwargs):
    data1 = {"a": a, "b": b}
    return (
        {k: v for k, v in data1.items() if v is not None},
        {k: v for k, v in kwargs.items() if v is not None},
    )

Because this function was defined to capture all of **kwargs into a specific area of functionality, it's not possible to add c: int | None = None as an item to include in the data1 component.

Avoiding **kwargs ensures that we don't find ourselves in this sort of unfortunate situation.


📚 Documentation preview 📚: https://globus-sdk-python--775.org.readthedocs.build/en/775/

@sirosen sirosen added the no-news-is-good-news This change does not require a news file label Jul 6, 2023
@sirosen sirosen merged commit 3339d75 into globus:main Jul 6, 2023
14 of 15 checks passed
@sirosen sirosen deleted the add-contrib-kwargs-guidance branch July 6, 2023 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants