Added Collaborator, Contributor and Repository.collaborators(affiliation) #731

AndreasBackx · 2017-09-18T17:00:25Z

See #730 for some background information. This still fails 1 test because the cassette Repository_collaborators.json needs to be updated with the permissions as it somehow does not contain that information. The GitHub API does say that it should be there, but the recording is from 2014 so perhaps it wasn't returned back then.

Note that I have currently put the named parameter affiliation at the front of the arguments in Repository.collaborators. I did this to be in line with the other similar iterators. This can be moved to the back, but I assume some backwards-incompatible changes are fine for 1.0.0?

This uses separate classes for the specific User object uses as I understood the idea was from #670.

AndreasBackx · 2018-04-04T22:43:41Z

@sigmavirus24 any chance this gets looked at?

sigmavirus24 · 2018-04-05T23:01:42Z

@AndreasBackx I'd be happy to give this a look. Would you mind rebasing this to merge cleanly?

AndreasBackx · 2018-04-06T09:24:05Z

I will, I'm currently not at my apartment for a few days so I might not be able to get to it this week. I'll rebase when I get back to my apartment, I hope to be back in a couple days.

omgjlk

Couple comments inline. Also I'd like to know what the right method is to go from having a Collaborator object and turning it into a User object.

omgjlk · 2018-04-06T22:16:15Z

github3/repos/repo.py

@@ -524,17 +524,24 @@ def code_frequency(self, number=-1, etag=None):
        url = self._build_url('stats', 'code_frequency', base_url=self._api)
        return self._iter(int(number), url, list, etag=etag)

-    def collaborators(self, number=-1, etag=None):
+    def collaborators(self, affiliation='all', number=-1, etag=None):


The affiliations parameter should be documented here.

omgjlk · 2018-04-06T22:18:37Z

github3/repos/repo.py

        """
        url = self._build_url('collaborators', base_url=self._api)
-        return self._iter(int(number), url, users.ShortUser, etag=etag)
+        affiliations = {'outside', 'direct', 'all'}


This seems like a static list, not sure it belongs defined in this function or if it should be defined somewhere more global. That's likely a project preference thing, I defer to @sigmavirus24 .

Eh. I've waffled on this before. I think we do something similar in other places. I've wondered about having a github3.constants or (github3._constants) but it never felt important enough, honestly.

I also toyed with some decorators that would do strong validation of the parameters and what's passed in to the function but that turned out to be way too much work.

We can create an enum Affiliation perhaps with uppercase choices for each: Affiliation.OUTSIDE. You can then do list(Affiliation) for a list of all of them (though it'll give a list of enums, not their values). Maybe something along those lines could be adopted for other stuff as well. I don't really mind here. Tell me what to do for the constants here, and it'll get done.

I don't think you need to tackle this @AndreasBackx. I think this is a murky corner for a library that supports Py2 and Py3. If we were Py3 only, I'd go whole-hog on using an Enum. Relying on the enum34 backport, however, has its own issues, I've found, and isn't worth the headache for end users right now.

Ah yes, I've been working exclusively in Python 3 so I've been forgetting about the Python 2 support as of late. Are there any plans to drop Python 2 support in the future?

Nothing firm. Maybe later this year. Maybe next year.

omgjlk · 2018-04-06T22:19:58Z

github3/repos/users.py

+    This represents a user as a collaborator of a :class:`Repository`.
+    This adds permission information of the user to the repository.
+
+    .. versionadded:: 1.0.0


This version is a bit behind now.

sigmavirus24 · 2018-04-06T23:38:04Z

github3/repos/users.py

@@ -0,0 +1,33 @@
+from ..users import _User


Is there reason to not just put these classes in github3.users?

No reason at all, preference perhaps. It'll get changed.

Oh wait, now that I think about it. The reason I put it in github3.repos.users instead of github3.users is because I assumed that was implied in issue #670. As a collaborator's endpoint is /repos/:owner/:repo/collaborators. They belong to the repo so I thought it would be good to put those in github3.repos.users instead. Regardless though, tell me what you think is best.

I think there's precedence for both decisions. I think we already have Contributor in github3.users so moving Collaborator in there too probably wouldn't hurt.

sigmavirus24 · 2018-04-06T23:38:54Z

github3/repos/repo.py

        """
        url = self._build_url('collaborators', base_url=self._api)
-        return self._iter(int(number), url, users.ShortUser, etag=etag)
+        affiliations = {'outside', 'direct', 'all'}


Eh. I've waffled on this before. I think we do something similar in other places. I've wondered about having a github3.constants or (github3._constants) but it never felt important enough, honestly.

sigmavirus24 · 2018-04-06T23:39:42Z

github3/repos/repo.py

        """
        url = self._build_url('collaborators', base_url=self._api)
-        return self._iter(int(number), url, users.ShortUser, etag=etag)
+        affiliations = {'outside', 'direct', 'all'}


I also toyed with some decorators that would do strong validation of the parameters and what's passed in to the function but that turned out to be way too much work.

sigmavirus24 · 2018-04-06T23:40:30Z

github3/repos/repo.py

+                "'direct', or 'all' (default 'all')."
+            )
+        params = {'affiliation': affiliation}
+        return self._iter(int(number), url, Collaborator, params, etag=etag)


I think as a rule we've switched to importing modules and not classes, so these shouldn't be the exception.

sigmavirus24 · 2018-04-06T23:43:03Z

github3/repos/repo.py

+        affiliations = {'outside', 'direct', 'all'}
+        if affiliation not in affiliations:
+            raise ValueError(
+                "Invalid affiliation parameter passed, must be 'outside', "


I wonder if we should format the given affiliation into this to make it easier for debugging. Perhaps:

("Invalid affiliation value {!r} parameter passed, must be 'outside'," "'direct', or 'all' (defaults to 'all').").format(affiliation)

sigmavirus24 · 2018-04-06T23:44:58Z

tests/unit/test_repos_repo.py

+        )
+
+    def test_collaborators_valid_affiliation(self):
+        """


I know we're not great at this elsewhere in the tests, but I'm hoping to get our doc-strings cleaned up soon. I tried to do so in the code itself. I'm going to target the tests next and it'd be good to do less work 😉

Could you substitute something like

"""Verify we can list collaborator with a valid affiliation. Let's ensure that our affiliation validation works. """

Probably a mistake on my end. I usually follow the same principle.

sigmavirus24 · 2018-04-06T23:45:45Z

Gave this a review pass as well. There's no rush to fix it up. This looks really close to perfect. Thanks @AndreasBackx for putting the work into doing this. 🎉

AndreasBackx · 2018-04-07T10:01:18Z

Alright, I've looked over the comments and I'll fix what needs to be fixed and I've replied to your questions. I'll try to get to it the coming days. Thanks for taking the time to review the PR.

sigmavirus24 · 2018-04-07T13:45:42Z

Oh, and regarding how Contributor and Collaborator turn into a User, I think the answer lies in the examples of how ShortUser becomes a User(i.e., setting _refresh_to)

AndreasBackx · 2018-04-08T10:14:23Z

I'm rebasing right now and see that Contributor has the attribute contributions, but it seems to be documented as contributions_count. Is the documentation wrong or should the attribute be renamed?

A User has got:

self.followers_count = user['followers']

And the Contributor has got:

class Contributor(_User):
    """Object for the specialized representation of a contributor.

    When retrieving a repository's contributors, GitHub returns the same
    information as a :class:`~github3.users.ShortUser` with an additional
    attribute:

    .. versionadded:: 1.0.0

        This class was added in version 1.0.0

    .. attribute:: contributions_count

        The number of contributions a contributor has made to the repository

    """

    class_name = 'Contributor'

    def _update_attributes(self, contributor):
        super(Contributor, self)._update_attributes(contributor)
        self.contributions = contributor['contributions']

Should it become:

self.contributions_count = contributor['contributions']

I thought I'd post it here and include it in the PR perhaps, it seems a bit silly to make a new PR/issue just for this.

…ion). See sigmavirus24#730 for some background information. Note that I have currently put the named parameter `affiliation` at the front of the arguments in `Repository.collaborators`. I did this to be in line with the other similar iterators. This can be moved to the back, but I assume some backwards-incompatible changes are fine for 1.0.0? This uses separate classes for the specific User object uses as I understood the idea was from sigmavirus24#670.

AndreasBackx · 2018-04-08T12:05:39Z

I've rebased the PR following your feedback. I haven't changed Contributor.contributions to Contributor.contributions_count like I mentioned in my previous comment.

I've added the ability to refresh the Collaborator as well like @sigmavirus24 mentioned and added it to the docs. I assumed that this will be in 1.0.3 so I've used that version in the version changes/additions.

I had to update the cassette Repository_collaborators, but because I don't have push access to this repository, which is used in the tests and is required by the GitHub API for retrieving the collaborators. I changed it temporarily to my PR's develop branch, ran the tests, and changed the username from AndreasBackx to sigmavirus24. It shouldn't make a difference for the tests that are performed using it or in the future I assume.

Let me know if I missed something.

PS: The AppVeyor build seems to use the old commit, could someone run the latest? I assume Travis is running as I am typing this, but I can't check as it seems to be set to private (which I assume is in case of secrets leaking?). I tested everything successfully locally.

sigmavirus24 · 2018-04-08T14:01:23Z

It should be contributions_count that's what I intended it to be at least :)

Reverted Repository_collaborators.json, as the base64 in it still contained the wrong URLs for the integration tests. They need to be regenerated by someone who has push access to the repository.

sigmavirus24 · 2018-04-08T15:06:46Z

Let me try closing and re-opening this to trigger builds. Travis was broken so let's see if this fixes AppVeyor too.

AndreasBackx · 2018-04-08T15:10:41Z

I've reverted Repository_collaborators.json to its original state as the base64 strings in it contain the URLs to my repository. It needs to be regenerated by someone who has push access to this repository. I think I forgot to test the file after I changed everything, and it was failing because the library received my fork's URL instead of this repository's URL. But betamax had this repository's URL in the cassette so it failed.

I've also found another "bug" as you will in the tox config:

envlist = py{27,34,35,36,py},py{27,34,36}-flake8,docstrings

The envlist mentions a py36-flake8 environment, but that one does not exist so it falls back to py36 which then essentially gets run twice when running tox.

sigmavirus24 · 2018-04-08T15:39:05Z

I re-recorded the cassette and pushed it to your branch. @AndreasBackx can you send another PR to add that environment to our tox.ini?

All feedback seems to have been addressed. Thanks for reviewing this

sigmavirus24 · 2018-04-08T15:47:18Z

I pushed some extra changes here as I noticed some things I didn't quite like about the docstrings.

sigmavirus24 · 2018-04-08T16:57:24Z

🎉 Thanks @AndreasBackx!

AndreasBackx · 2018-04-08T22:49:29Z

Regarding the environment change in tox.ini do I need to add it or do I need to change the py34-flake8 to py36-flake8? I doubt that we need 2 of those environments for Python 3?

PS: I accidentally checked tests/cassettes/Repository_collaboratorsold.json in. That was my mistake.

sigmavirus24 · 2018-04-09T11:39:04Z

You can change py34 to py36 for Flake8. Just make sure you remove the py34 from the envlist.

And don't worry about checking in the old cassette. It's all sorted 😄

AndreasBackx · 2018-04-09T13:28:39Z

Will do.

I know, I saw the diff which is when I realised that I had accidentally checked it in.

See sigmavirus24#731.

This also requires us to be slightly more explicit with our Travis configuration. See discussion on gh-731.

omgjlk previously requested changes Apr 6, 2018

View reviewed changes

sigmavirus24 requested changes Apr 6, 2018

View reviewed changes

AndreasBackx force-pushed the feature/collaborators branch from 15af1b9 to ff8501a Compare April 8, 2018 11:52

AndreasBackx force-pushed the feature/collaborators branch from ff8501a to 17de8b8 Compare April 8, 2018 12:00

Renamed Contributor.contributions_count.

7df6440

Reverted Repository_collaborators.json, as the base64 in it still contained the wrong URLs for the integration tests. They need to be regenerated by someone who has push access to the repository.

sigmavirus24 closed this Apr 8, 2018

sigmavirus24 reopened this Apr 8, 2018

Re-record cassette and remove "archived" copy

65c1833

Fix-up docstrings for Collaborators work

a3a28fb

sigmavirus24 approved these changes Apr 8, 2018

View reviewed changes

sigmavirus24 merged commit 05ed0c6 into sigmavirus24:develop Apr 8, 2018

AndreasBackx added a commit to AndreasBackx/github3.py that referenced this pull request Apr 9, 2018

Removed py34-flake8 and added py36-flake8.

41876f7

See sigmavirus24#731.

AndreasBackx mentioned this pull request Apr 9, 2018

Removed py34-flake8 and added py36-flake8. #822

Merged

sigmavirus24 pushed a commit that referenced this pull request Apr 9, 2018

Replace py34-flake8 with py36-flake8

0dea703

This also requires us to be slightly more explicit with our Travis configuration. See discussion on gh-731.

Added Collaborator, Contributor and Repository.collaborators(affiliation) #731

Added Collaborator, Contributor and Repository.collaborators(affiliation) #731

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!