Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing minor code quality issues #231

Merged
merged 4 commits into from
Mar 16, 2018

Conversation

joshcanhelp
Copy link
Contributor

Ran through Code Inspector in PhpStorm and PHP CodeSniffer

@joshcanhelp joshcanhelp added the WIP label Mar 6, 2018
@joshcanhelp joshcanhelp changed the base branch from master to 5.x.x-dev March 6, 2018 17:26
@joshcanhelp joshcanhelp added this to the v5-Next milestone Mar 6, 2018
@@ -4,8 +4,6 @@
[![Build Status](https://travis-ci.org/auth0/auth0-PHP.png)](https://travis-ci.org/auth0/auth0-PHP)
[![Code Climate](https://codeclimate.com/github/auth0/auth0-PHP/badges/gpa.svg)](https://codeclimate.com/github/auth0/auth0-PHP)
[![Test Coverage](https://codeclimate.com/github/auth0/auth0-PHP/badges/coverage.svg)](https://codeclimate.com/github/auth0/auth0-PHP/coverage)
[![Dependencies](https://www.versioneye.com/php/auth0:auth0-php/badge.svg)](https://www.versioneye.com/php/auth0:auth0-php)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1,4 +1,13 @@
<?php
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summary of changes:

  • Spacing and indenting (automatically with phpcbf)
  • Docblocks
  • Private properties get a _ prefix
  • Line length = 85

@@ -49,7 +49,7 @@ public function testAll() {

$this->afterCreate($created);

$client3 = $client->update($this->getId($created), $this->getUpdateBody());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used

@joshcanhelp joshcanhelp changed the title [WIP] Fixing minor code quality issues Fixing minor code quality issues Mar 6, 2018
@@ -4,8 +4,6 @@
[![Build Status](https://travis-ci.org/auth0/auth0-PHP.png)](https://travis-ci.org/auth0/auth0-PHP)
[![Code Climate](https://codeclimate.com/github/auth0/auth0-PHP/badges/gpa.svg)](https://codeclimate.com/github/auth0/auth0-PHP)
[![Test Coverage](https://codeclimate.com/github/auth0/auth0-PHP/badges/coverage.svg)](https://codeclimate.com/github/auth0/auth0-PHP/coverage)
[![Dependencies](https://www.versioneye.com/php/auth0:auth0-php/badge.svg)](https://www.versioneye.com/php/auth0:auth0-php)
[![HHVM Status](http://hhvm.h4cc.de/badge/auth0/auth0-php.svg)](http://hhvm.h4cc.de/package/auth0/auth0-php)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not workin'

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

So monster PR, looks fine in general.

@@ -6,6 +6,8 @@

class BlacklistsTest extends ApiTests {

private $domain;
Copy link
Member

Choose a reason for hiding this comment

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

How come no _ for this private property, or tests out of scope in this refactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cocojoe - I ran the phpcs/phpcbf against all the files and the list was so long that my terminal couldn't show it all 😆 so I just used it on src/API/Authentication.php (inspected the rest) to see how much work as involved. Answer is: a lot but I think it's worth it. I would like to see 5.x.x "finished" before it's archived and I see this as part of that.

@joshcanhelp
Copy link
Contributor Author

@cocojoe - Approved?

@joshcanhelp joshcanhelp modified the milestones: v5-Next, 5.1.1 Mar 13, 2018
Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

LGTM

@joshcanhelp joshcanhelp merged commit a9b45c9 into 5.x.x-dev Mar 16, 2018
@joshcanhelp joshcanhelp deleted the fixed-minor-code-quality-issues branch March 16, 2018 15:56
@joshcanhelp joshcanhelp mentioned this pull request Apr 3, 2018
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants