JGithub Improvements. Labels management #1124

Closed
wants to merge 14 commits into from

4 participants

@nprasath002

Added some methods to manage labels in Github

  • createLabel
  • editLabel
  • deleteLabel
  • getLabel
  • getLabels
@ianmacl ianmacl was assigned Apr 8, 2012
@ianmacl

Code looks reasonable. Commits needs to be amended so that your proper email address is used (Github isn't picking up your identity correctly for the commits). Also, tests need to be written for these added methods.

@realityking
Joomla! member

@nprasath002 Could you fix these as per Ian's comment. Would be great if you could rebase them into one commit as well.

@realityking realityking commented on the diff Jul 18, 2012
libraries/joomla/github/http.php
@@ -46,8 +46,9 @@ class JGithubHttp extends JHttp
*/
public function __construct(JRegistry $options = null, JHttpTransport $transport = null)
{
- // Call the JHttp constructor to setup the object.
- parent::__construct($options, $transport);
@realityking
Joomla! member

Why are you removing this? This increases the chance of breaking things when something is added to the JHttp constructor that is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@realityking realityking commented on the diff Jul 18, 2012
libraries/joomla/github/issues.php
@@ -107,6 +107,45 @@ public function createComment($user, $repo, $issueId, $body)
return json_decode($response->body);
}
+ /**
+ * Method to create a label on a repo.
+ *
+ * @param string $user The name of the owner of the GitHub repository.
+ * @param string $repo The name of the GitHub repository.
+ * @param string $name The label name.
+ * @param string $color The label color.
+ *
+ * @return object
+ *
+ * @since 12.1
@realityking
Joomla! member

Please change these to 12.2.

And by that he means 12.3 ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@LouisLandry

@nprasath002 would love to get this merged into 12.3 if you have the time for the little cleanup necessary.

@LouisLandry

OK, @nprasath002 this has been marked for including in the 12.3 release (about 2 months out). If you can find the time to make the adjustments listed above we'd love to merge it. If you have questions about how to do any of it please feel free to either ask in this thread, or ask on the platform mailing list.

For now I'm going to close this pull request until you've got it cleaned up and ready for further review. When you get that done please re-open it and we'll get it sorted out then. Thanks a bunch for the work on this, it will prove very useful!

@LouisLandry LouisLandry closed this Oct 9, 2012
@nprasath002

I cleanedup and made a new pull request.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment