Skip to content

Conversation

@herlo
Copy link

@herlo herlo commented Dec 16, 2011

I figure this functionality might be useful. Let me know if you would like anything cleaned up before submitting.

@JNRowe
Copy link
Collaborator

JNRowe commented Dec 16, 2011

Thanks, looks great!

I'm unable to work on this right now, but we'll get it merged over the weekend.

A couple of quick comments:

  1. Could do with some tests
  2. There are a few commented debugging statements left in that should probably be removed

Neither of which are show stoppers, and I'll fix it up before merging... if you've not done so by then ;)

@JNRowe
Copy link
Collaborator

JNRowe commented Dec 19, 2011

Actually, looks like this can't be merged in its current state. On top of the issues from my previous comment

  • Neither new method return the correct objects, Team for add_team and a list of Users for add_member
  • add_team doesn't work with multiple repos, as it overwrites dictionary keys on each iteration.

@JNRowe
Copy link
Collaborator

JNRowe commented Dec 19, 2011

I rewrote the functionality in JNRowe/python-github2@feat/gooseprojects_teams, it fixes the problems I mentioned in the earlier comments.

It also changes the functionality slightly, as GitHub specify qualified project names when adding teams we use that in the add_team method.

Probably needs some more testing...

@herlo
Copy link
Author

herlo commented Dec 20, 2011

Seems fine to me. If you cannot tell, I wasn't sure what the datatype= actually accomplished. It's a bit clearer here and I will try to make sure to include the proper return datatype in the future. Glad to have helped, even if just to get the concept into code.

@JNRowe
Copy link
Collaborator

JNRowe commented Dec 20, 2011

If you cannot tell, I wasn't sure what the datatype= actually accomplished.

Yep, that really should be documented somewhere.

If you are playing with the code and find other things you just cannot get your head around feel free to open an issue. Documentation tends to get written as someone points out a gap ;)

Glad to have helped, even if just to get the concept into code.

A pull request is always a great help, even if it has a couple of problems. Thanks for that.

I'll play with these changes some more later, just to make sure I haven't done something silly. And merge them later today if no problems turn up.

A new release is coming over the next few days, and these additions will be in it.

@JNRowe
Copy link
Collaborator

JNRowe commented Dec 20, 2011

Merged in 8b860ac.

Thanks, again.

@JNRowe JNRowe closed this Dec 20, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants