[5.0] Much verbose output in assertResponseStatus() method #8253

shehi · 2015-04-02T12:53:51Z

Much verbose output in assertResponseStatus() method, where the output tells what was expected, what is actual.

taylorotwell · 2015-04-02T18:38:00Z

src/Illuminate/Foundation/Testing/AssertionsTrait.php

@@ -14,7 +14,7 @@ public function assertResponseOk()
 	{
 		$actual = $this->response->getStatusCode();

-		return PHPUnit::assertTrue($this->response->isOk(), 'Expected status code 200, got ' .$actual);
+		return PHPUnit::assertTrue($this->response->isOk(), 'Expected status code 200, got ' . $actual);


Don't put spaces in between each dot.

With all due respect, are you going to reject people's PRs just because there is a bloody space in between dots? How do you recommend I put spaces around dots in the line below (I put spaces here for more readability and style-unity with the line I added)?! Does this project has some coding guidelines I missed? Because what you ask doesn't even relate to PSR guidelines. Correct me if I am wrong. I do bloody care about this project as much as any developer who uses it in a daily basis. Please show some understanding! :( Have a good day!

No, Taylor is asking you to change it.

Does this project has some coding guidelines I missed?

Our code style is self documented by our code. The same goes for Symfony.

@GrahamCampbell , @taylorotwell isn't 5 year old kid who can't speak for himself. Stop jumping into every convo mate. Reject the PR, I am sure those spaces mean more to this project than our contributions! And I don't care about Symfony, I am not sending this PR to Symfony.

@shehi Since you mentioned PSR guidelines, you probably know how important a consistent code style is for projects as large as this one.

Your PR looks good except for this small issue - it would be awesome if you could fix this. In fact, you can also interpolate the variables directly in a string with double quotes - that's also fine by Laravel's standards:

"Expected status code 200, got $actual"

@shehi Your PR was not rejected. On the contrary, the fact that Taylor asked you to fix the whitespace to match his style indicates that he wants to accept this change.

The laravel docs have some very basic coding guidelines, but it can't possibly cover everything. Anything that is not covered there, can be understood from reading Laravel's source code. If anyone sends a PR where the format does not fully match Laravel's coding style, it is kindly pointed out to them, they change it, and then the PR can be accepted.

Also, keep in mind that Taylor is just one person and he can't possibly get to everything himself. @GrahamCampbell has been generously giving much of his own time to help Taylor out with fielding issues and pull requests, so that Taylor can focus on the actual code.

P.S. you don't have to submit another PR. You can update this one. After you make your changes, run this command:

git commit -a --amend --no-edit git push -f origin patch-1

This will update the current PR with the changes you've made.

@JosephSilber , original line already had space to the left of dot. I just added one to its right. It already had so called standards "violated". Anyway, let's get this over with. Fixed! Now the space to the left is deleted. Don't bloody ask me to put it back in! :)

Yeh, none of us get paid for Laravel, though Taylor has an income for Laravel related products.

Our code style is self documented by our code. The same goes for Symfony.

This is not entirely precise for Symfony. We do have a document explaining our code standard rules.

That document is incomplete though. I work on php-cs-fixer, and changes are made to symfony's official coding standards by us merging cs changes into symfony/symfony. Our code gets used in fabbot.io which checks all symfony pull requests. That's why I said symfony's code is selfdocumenting - because it actually is.

GrahamCampbell · 2015-04-02T20:50:43Z

src/Illuminate/Foundation/Testing/AssertionsTrait.php

@@ -25,7 +25,7 @@ public function assertResponseOk()
 	 */
 	public function assertResponseStatus($code)
 	{
-		return PHPUnit::assertEquals($code, $this->response->getStatusCode());
+		return PHPUnit::assertEquals($code, $this->response->getStatusCode(), 'Expected status code '.$code.', got '.$actual);


"Expected status code $code, got $actual." would be much better btw.

Wait until we get some complex objects interpolated into strings, then you won't sound so funny :) I will do it as you ask (noticed inside the same file, down below, everything is inside the string, instead of being concat'ed).

@shehi That would been an issue anyway. PHP would have behaved the same when you try to concatenate them.

GrahamCampbell · 2015-04-02T21:02:15Z

src/Illuminate/Foundation/Testing/AssertionsTrait.php

@@ -14,7 +14,7 @@ public function assertResponseOk()
 	{
 		$actual = $this->response->getStatusCode();

-		return PHPUnit::assertTrue($this->response->isOk(), 'Expected status code 200, got ' .$actual);
+		return PHPUnit::assertTrue($this->response->isOk(), "Expected status code 200, got $actual");


Shouldn't we end in a full stop?

Thought about that, but the original code didn't have that. And full stop (the dot) is a character by itself: in a context where you compare the expected with the actual, I think introducing additional chars would be a bad idea.

GrahamCampbell · 2015-04-02T21:02:34Z

This looks much better now, thanks. Could you squash to one commit at the end please?

shehi · 2015-04-02T21:07:20Z

@GrahamCampbell , I am editing code directly on GitHub. Didn't bother to fork it locally. Is there a way to squash it over here?

shehi · 2015-04-02T21:09:17Z

@GrahamCampbell : feel free to squash it even if it means the change of author. Or wait til tomorrow, its midnight here right now. Or decline this PR, I will create new PR quickie and send in.

GrahamCampbell · 2015-04-02T21:10:48Z

We can technically squash on merge, but that rarely happens.

shehi · 2015-04-02T21:12:10Z

Please do, as long as we don't damage anything, its fine by me :)

arrilot · 2015-04-03T15:23:54Z

src/Illuminate/Foundation/Testing/AssertionsTrait.php

@@ -25,7 +25,7 @@ public function assertResponseOk()
 	 */
 	public function assertResponseStatus($code)
 	{
-		return PHPUnit::assertEquals($code, $this->response->getStatusCode());
+		return PHPUnit::assertEquals($code, $this->response->getStatusCode(), "Expected status code $code, got $actual");


$actual is not degined here.

Fixed. @GrahamCampbell could you please merge this by squashing it in the process? Authorship isn't really important for such a minor enhancement. Thanks.

I can squash and still keep your authorship on the commit.

@GrahamCampbell ok, just do it :)

shehi · 2015-04-06T10:15:07Z

@taylorotwell , @GrahamCampbell : please merge by squashing. PR author-ship isn't really important for this tiny piece of code. Or just decline and add the code in your next update yourself. Thanks.

[5.0] Much verbose output in assertResponseStatus() method

Much verbose output in assertResponseStatus() method

149824c

Much verbose output in assertResponseStatus() method, where the output tells what was expected, what is actual.

taylorotwell reviewed Apr 2, 2015
View reviewed changes

GrahamCampbell added low priority labels Apr 2, 2015

GrahamCampbell changed the title ~~Much verbose output in assertResponseStatus() method~~ [5.0] Much verbose output in assertResponseStatus() method Apr 2, 2015

Update AssertionsTrait.php

e631658

GrahamCampbell reviewed Apr 2, 2015
View reviewed changes

shehi added 2 commits April 2, 2015 23:58

Update AssertionsTrait.php

874b625

Update AssertionsTrait.php

9dc7fc5

GrahamCampbell reviewed Apr 2, 2015
View reviewed changes

arrilot reviewed Apr 3, 2015
View reviewed changes

Update AssertionsTrait.php

0ce8c18

taylorotwell added a commit that referenced this pull request Apr 12, 2015

Merge pull request #8253 from shehi/patch-1

7c432c8

[5.0] Much verbose output in assertResponseStatus() method

taylorotwell merged commit 7c432c8 into laravel:5.0 Apr 12, 2015

[5.0] Much verbose output in assertResponseStatus() method #8253

[5.0] Much verbose output in assertResponseStatus() method #8253

Uh oh!

Conversation

shehi commented Apr 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrahamCampbell commented Apr 2, 2015

Uh oh!

shehi commented Apr 2, 2015

Uh oh!

shehi commented Apr 2, 2015

Uh oh!

GrahamCampbell commented Apr 2, 2015

Uh oh!

shehi commented Apr 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shehi commented Apr 6, 2015

Uh oh!

Uh oh!