Skip to content

Conversation

jpastoor
Copy link
Collaborator

Update of #25.

  • Moved logic from constructor to setEndpoint
  • Changed regex to rtrim
  • Wrote unit test
  • Rebased on current master

src/Jira/Api.php Outdated
{
$this->fields = array();

//Regular expression to remove trailing slash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the following:

  1. add a space after //
  2. add a dot at the end of the comment text

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 1, I'm no fan of 2

@aik099
Copy link
Member

aik099 commented Feb 28, 2016

Today’s review completed. When done making changes please add inline comment in each of them.

Added regular expression to remove trailing slash if included in URL. Api::__construct

use chobie\Jira\Api\Authentication\Basic;

class Jira_Api_Authentication_BasicTest extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you namespace test classes why not rename Jira_Api_Authentication_BasicTest into BasicTest to make autoloading work?

It works currently, because PHPUnit looks for all PHP files that ends up with Test.php and executes them without using Composer's autoloader.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@aik099
Copy link
Member

aik099 commented Feb 28, 2016

And you can safely squash when you're done fixing.

@aik099
Copy link
Member

aik099 commented Feb 28, 2016

Merging, thanks @jpastoor .

aik099 pushed a commit that referenced this pull request Feb 28, 2016
Remove trailing slash in URL if it exists
@aik099 aik099 merged commit 2d8969d into console-helpers:master Feb 28, 2016
@jpastoor jpastoor deleted the Procta-master branch February 29, 2016 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants