EnterpriseyIntranet / nextcloud-API

NextCloud OCS API for Python
GNU General Public License v3.0
27 stars 27 forks source link

Fix, refactor Requester, User, Apps, Groups, add tests, docstrings #8

Closed danil-topchiy closed 5 years ago

danil-topchiy commented 5 years ago

Fix not working methods, refactoring to pep8 standards, writing tests for fixed methods

matejak commented 5 years ago

Good start, but rebase against the mastre branch - the PR will become simpler to review.

matejak commented 5 years ago

Good, now it's nicely visible what is going on. I like it so far.

danil-topchiy commented 5 years ago

@matejak commit ef7e6b5561aca58d132af68f51e87c790ee213fc, made a quick fix for a critical bug with Requester instance shared between all groups of requests (Users, Groups, Share...), every class was inheriting from WithRequester where in initialization method it took requester as argument and set API_URL as self.API_URL (individual for every class).

class WithRequester(object):
    def __init__(self, requester):
        self.requester = requester
        self.requester.API_URL = self.API_URL

So all this instances were created in NextCloud class initialization and shared single Requester instance, and as a result Requester has it's base_url where it made requests to, from the last instantiated class and only that class had working requests to right url.

class NextCloud(object):

    def __init__(self, endpoint, user, passwd, js=False):
        self.query_components = []

        requester = Requester(endpoint, user, passwd, js)

        self.functionality = {
            "Apps": Apps(requester),
            "Group": Group(requester),
            "GroupFolders": GroupFolders(requester),
            "Share": Share(requester),
            "User": User(requester),
        }

Right now fixed it with class property for WithRequester class where dynamically setting API_URL to requester

    @property
    def requester(self):
        """ Get requester instance """
        # dynamically set API_URL for requester
        self._requester.API_URL = self.API_URL
        return self._requester

but I think it's temporary fix and I should rewrite it so Requester would accept API_URL to every request and return bounded method. What do you think about it?

matejak commented 5 years ago

Great that you have noticed the critical issue! I like the direction where this PR is going. Here are some remarks:

I suggest that you polish this PR according to your judgement, and I merge it, so the work can continue in subsequent PRs. Good job!

danil-topchiy commented 5 years ago

Thanks for review! Added newlines to test, and in future will use google docstrings to comments methods.

Will continue with fixes, docs and refactoring for Shares, GroupFolders in another PR.

matejak commented 5 years ago

Great, I appreciate your PR!