Skip to content

Conversation

@djagya
Copy link
Contributor

@djagya djagya commented Feb 27, 2016

Also one test is removed, because my opinion is that strong-typing is better than condition with 'instanceof', especially in cases when it is a simple check for only one class.
And for strong-typing we don't need tests, because it's like test for builtin php functionality, what is already tested.

Also I removed @desc tag, because phpDoc doesn't have that tag implemented and it does nothing.

Also I make code following PSR-2 standard, but if you don't like that, I can return all those braces, that are now on the next line.

danil zakablukovskii added 4 commits February 27, 2016 17:13
removed @desc tag, since it doesn't exist in phpdoc
fixed comments
fixed namespaces
added strong-typing for some methods
@jasonrhodes
Copy link
Contributor

Thanks for the PR, @djagya—we'll take a look and get back to you with any comments/questions.


private static $utils;
private $adapterMock;
/** @var SparkPost */
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this commented line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just to specify that $resource has SparkPost object inside to remove some warnings in code (otherwise IDE doesn't know which type has $this->resource)

@jasonrhodes
Copy link
Contributor

So looking over this, I think we're going to implement http://cs.sensiolabs.org/ with PSR-2 style throughout the lib (unless we discover any reasons we can't) -- how hard would it be for you to back out the "style-only" changes so it's easier to see what this PR is doing, and then we'll run the style fixer on everything and release it... ? If it's a huge pain, we can just leave it in... it doesn't hurt anything except some clarity around what this PR is changing. Thanks again!

@djagya
Copy link
Contributor Author

djagya commented Feb 28, 2016

Sure, it's not a problem, I'll revert my changes styles changes back.

@djagya
Copy link
Contributor Author

djagya commented Feb 28, 2016

Done

@jasonrhodes
Copy link
Contributor

LGTM, I'll probably squash some of those commits but that's easy for me to take care of. Thanks so much for the work on this, @djagya! Closes #42

jasonrhodes added a commit that referenced this pull request Feb 28, 2016
Fixed namespaces, cleaned code/comments - closes #42
@jasonrhodes jasonrhodes merged commit 23378d2 into SparkPost:master Feb 28, 2016
@djagya djagya deleted the 42-fix-namespaces-clean-code branch February 28, 2016 21:58
@djagya
Copy link
Contributor Author

djagya commented Feb 28, 2016

@jasonrhodes sure, no problem!

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.

2 participants