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

[HttpClient] Add tests - update code style nits. #31018

Merged
merged 1 commit into from Apr 9, 2019

Conversation

Projects
None yet
5 participants
@drupol
Copy link
Contributor

drupol commented Apr 8, 2019

WIP - please discard this PR for now.

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31035
License MIT
Doc PR

@drupol drupol changed the title [wip] Httpclient small nits [wip] [symfony/HttpClient] small nits Apr 8, 2019

@drupol drupol force-pushed the drupol:httpclient-nits branch 6 times, most recently from 23ed723 to ae001ef Apr 8, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 8, 2019

@Nyholm
Copy link
Member

Nyholm left a comment

Thank you for this PR.

Im like that you fixed some bug here =)

There are a few places where I don't understand why your change bring any value. Could you write some notes to help me understand?

Show resolved Hide resolved src/Symfony/Component/HttpClient/CachingHttpClient.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/CachingHttpClient.php
Show resolved Hide resolved src/Symfony/Component/HttpClient/CachingHttpClient.php Outdated
@@ -109,7 +113,7 @@ public function request(string $method, string $url, array $options = []): Respo
$options['headers']['range'] ?? null,
];
if ('GET' === $method && !$options['body'] && $expectedHeaders === $pushedHeaders) {
if ('GET' === $method && $expectedHeaders === $pushedHeaders && !$options['body']) {

This comment has been minimized.

Copy link
@Nyholm

Nyholm Apr 8, 2019

Member

Could you also explain this?

This comment has been minimized.

Copy link
@drupol

drupol Apr 9, 2019

Author Contributor

Ah, I was pretty sure that such a question would be raised.

I'm using PHPStorm and an amazing plugin: PHP EA Extended, please, test it, I'm pretty sure you'll love it.
It gives you plenty of tips on how to improve the code you're writing.

In this exemple, it tells you that the condition could be optimized if we change the order of the conditions. Apparently, the fastest if should be first (less OPCODE) to gain in performance.
It's obviously micro-optimizations.

See the attached screenshot of the plugin on the original code.
Screenshot_20190409_083050

Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/MockHttpClient.php
Show resolved Hide resolved src/Symfony/Component/HttpClient/NativeHttpClient.php Outdated
@drupol

This comment has been minimized.

Copy link
Contributor Author

drupol commented Apr 9, 2019

Thanks for the comments, I will continue to reply asap.
I will push the fixes as well.

@drupol drupol force-pushed the drupol:httpclient-nits branch 2 times, most recently from 3308bf1 to e98a355 Apr 9, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

No objections: even if some changes are CS changes mostly, some are real fixes, good catch.
Note that this would have mostly been rejected on already released code as changing code style makes merging branches more difficult for mergers.

@Nyholm
Copy link
Member

Nyholm left a comment

I made a comment where I think the code should change.

I think there are some value in this PR. But at some places the change is just a matter of taste. I let other s decide for those instances.

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

one last comment on my side on the code
dont' forget to update the title of the PR (no need to use the "symfony/" prefix btw)
and you can add a comment with Status: needs review inside to flip the label once you are ready

@Nyholm

Nyholm approved these changes Apr 9, 2019

Copy link
Member

Nyholm left a comment

Im happy with these changes.

@drupol drupol changed the title [wip] [symfony/HttpClient] small nits [HttpClient] Add tests - update code style nits. Apr 9, 2019

@nicolas-grekas nicolas-grekas force-pushed the drupol:httpclient-nits branch from 9ec692c to e77108d Apr 9, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 9, 2019

Thank you @drupol.

@nicolas-grekas nicolas-grekas merged commit e77108d into symfony:master Apr 9, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Apr 9, 2019

bug #31018 [HttpClient] Add tests - update code style nits. (drupol)
This PR was squashed before being merged into the 4.3-dev branch (closes #31018).

Discussion
----------

[HttpClient] Add tests - update code style nits.

WIP - please discard this PR for now.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31035
| License       | MIT
| Doc PR        |

Commits
-------

e77108d [HttpClient] Add tests - update code style nits.

@drupol drupol deleted the drupol:httpclient-nits branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.