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

Consider renaming __constraints to __cloup_constraints__ #131

Closed
pocek opened this issue Nov 4, 2022 · 4 comments · Fixed by #132
Closed

Consider renaming __constraints to __cloup_constraints__ #131

pocek opened this issue Nov 4, 2022 · 4 comments · Fixed by #132
Labels
enhancement New feature or request
Milestone

Comments

@pocek
Copy link
Contributor

pocek commented Nov 4, 2022

Please consider renaming __constraints to __cloup_constraints__. Although at first it may sound like bikeshedding, they are valid reasons to do so:

  1. In classes, names like __name__ don't get mangled, but names like __name do (as described in https://docs.python.org/3/reference/expressions.html#private-name-mangling). It matters if you decorate a callback with a (callable) class, not just a function. And, if you do that, accessing __constraints for reasons like copying/moving them into another object gets tricky, because it's private. It's still possible though, but one has to use setattr/getattr/delattr family of functions instead of simple attribute access and del.

  2. It would be similar to what click does with __click_params__. With __click_params__, one can simply and happily copy/move them without resorting to setattr/getattr/delattr method.

  3. The name would be clearer - it's not obvious what __constraints are. I believe __cloup_constraints__ is simply a better name.

@pocek pocek added the bug Something isn't working label Nov 4, 2022
@janluke janluke added enhancement New feature or request and removed bug Something isn't working labels Nov 5, 2022
@janluke
Copy link
Owner

janluke commented Nov 5, 2022

Did this cause an actual problem to you? If yes, I'd be interested in reading an example.

@janluke
Copy link
Owner

janluke commented Nov 5, 2022

one can simply and happily copy/move them without resorting to setattr/getattr/delattr method.

I think I named it __constraints exactly to communicate that this is private API. But I think I'm okay with the change you're proposing. Do you want to open a PR?

@pocek
Copy link
Contributor Author

pocek commented Nov 5, 2022

Did this cause an actual problem to you? If yes, I'd be interested in reading an example.

Yes, it's a real problem I have encountered in a construct like this:

import cloup
from cloup.constraints import mutually_exclusive

class ClassBasedDecorator:
    def __init__(self, func):
        self.func = func

        # Standard attributes used by click
        self.__name__ = func.__name__
        self.__doc__ = func.__doc__

        if hasattr(func, '__click_params__'):
            self.__click_params__ = func.__click_params__
            del func.__click_params__

        # Cloup makes it harder
        if hasattr(func, '__constraints'):
            setattr(self, '__constraints',  getattr(func, '__constraints'))
            delattr(func, '__constraints')

    def __call__(self, *args, **kwargs):
        return self.func(*args, **kwargs)

@cloup.command(show_constraints=True)
@ClassBasedDecorator
@cloup.option('--foo')
@cloup.option('--bar')
@cloup.constraint(mutually_exclusive, ['foo', 'bar'])
def hello(foo, bar):
    """Hello"""
    print('hello')

hello()

I hope it doesn't look too unconventional - to have the decorators ordered like this, but this is the way that makes sense in my application.

Do you want to open a PR?

OK, I can to that.

@janluke
Copy link
Owner

janluke commented Nov 5, 2022

Yes, it's a real problem I have encountered in a construct like this. [...] I hope it doesn't look too unconventional - to have the decorators ordered like this, but this is the way that makes sense in my application.

The construct was already clear but I was curious about the real use case which requires you to mess with __constraints. Anyway, it's fine, don't feel obliged to share the details. I'll wait for the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants