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

Possible API improvements #5

Closed
sscherfke opened this issue Aug 21, 2017 · 4 comments
Closed

Possible API improvements #5

sscherfke opened this issue Aug 21, 2017 · 4 comments

Comments

@sscherfke
Copy link
Contributor

I like that you divided mm-driver’s functionality into several classes that we can compose and customize.

However, I think that the actual usage of the diver when calling the enpoint’s methods could be improved, so that instead of doing

mm = Driver(...)
mm.login()
mm.api['users'].get_user_by_username('another.name')

we can just do:

mm = MatterMost(...)
mm.login()
mm.users.get_user_by_username('another.name')

This is relatively easy to implement in another wrapper class (maybe it can even be merged into Driver):

class MatterMost:
    def __init__(self):
        self._driver = Driver()
        # self._driver.login()

    @property
    def userid(self):
        return self._driver.client.userid

    @property
    def username(self):
        return self._driver.client.username

    def __getattr__(self, key):
        try:
            return self._driver.api[key]
        except KeyError:
            raise AttributeError(key) from None

@Vaelor What do you think about this?

@Vaelor
Copy link
Owner

Vaelor commented Aug 23, 2017

Hey! Sorry for the late and short reply! :-D
Yes, that sounds like a great idea. A wrapper is maybe not a bad idea, I do like that stuff like userid, username belongs to Mattermost - I guess it would improve autocompletion, too?

@sscherfke
Copy link
Contributor Author

Autocompletion for what exactly?

But maybe we can improve auto-completion (at least for interactive session) by explicitly setting the API endpoints as attributes instead of looking them up dynamically:

class MatterMost:
    def __init__(self):
        self._driver = Driver()
        # self._driver.login()  # Useful for us, but maybe not for other users
        for k, v in self._driver.api.items():
            assert not hasattr(self, k)
            setattr(self, k, v)

    @property
    def userid(self):
        return self._driver.client.userid

    @property
    def username(self):
        return self._driver.client.username

@sscherfke
Copy link
Contributor Author

On the other hand, pylint will not complain about missing attributes in the __getattr_ variant, but in the setattr() variant, because it's not clever enough (and probably, it can’t) to find out which attributes I set in __init__().

@Vaelor
Copy link
Owner

Vaelor commented Sep 1, 2017

Well I like it either way. If autocompletion improves it is a nice plus, but not having to do api['users'] would be very nice in every case :-D If you want to implement it, go ahead. I currently have a lot of work at my paid workplace so I wouldn't get to it before mid september.

@Vaelor Vaelor closed this as completed in 33de668 Nov 26, 2017
Vaelor added a commit that referenced this issue Jan 16, 2018
 - Deprecated `Driver.api['']` calls.
 - Use `warnings` and `DeprecationWarning` instead of logger

 fix #5
Vaelor added a commit that referenced this issue Feb 22, 2018
 - Deprecated `Driver.api['']` calls.
 - Use `warnings` and `DeprecationWarning` instead of logger

 fix #5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants