Skip to content

Small code improvements#286

Merged
WyriHaximus merged 1 commit intoreactphp:masterfrom
seregazhuk:small-code-improvements
Dec 29, 2017
Merged

Small code improvements#286
WyriHaximus merged 1 commit intoreactphp:masterfrom
seregazhuk:small-code-improvements

Conversation

@seregazhuk
Copy link
Contributor

@seregazhuk seregazhuk commented Dec 13, 2017

This PR provides some small code improvement:

  • removes unused private properties
  • removes redundant fully-specified class name
  • removes arguments that are identical to their default values
  • adds missed class imports
  • removes unnecessary type casting

@WyriHaximus WyriHaximus added this to the v0.8.1 milestone Dec 13, 2017
clue
clue requested changes Dec 28, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Good catch and thank you for filing this PR! 👍 Can you update the comments below and maybe squash this to a reasonable number of commits?

src/Io/ServerRequest.php Outdated
use Psr\Http\Message\StreamInterface;
use Psr\Http\Message\UriInterface;
use RingCentral\Psr7\Request;
use InvalidArgumentException;
Copy link
Member

@clue clue Dec 28, 2017

Choose a reason for hiding this comment

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

It looks like these imports aren't used anywhere?

Copy link
Contributor Author

@seregazhuk seregazhuk Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, constructor has @throws annotation. My IDE (PHPStorm) highlights it with undefined class InvalidArgumentException. I've removed import and added root namespace to annotation.

src/Io/CloseProtectionStream.php
* */
class CloseProtectionStream extends EventEmitter implements ReadableStreamInterface
{
private $connection;
Copy link
Member

@clue clue Dec 28, 2017

Choose a reason for hiding this comment

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

Good catch! Apparently, this should be replaced with $input which is used in the constructor 👍

Copy link
Contributor Author

@seregazhuk seregazhuk Dec 28, 2017

Choose a reason for hiding this comment

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

done

@seregazhuk seregazhuk force-pushed the small-code-improvements branch from e03f56a to 742a015 Compare December 28, 2017 14:42
@seregazhuk seregazhuk force-pushed the small-code-improvements branch from 742a015 to ceba730 Compare December 28, 2017 14:50
@seregazhuk
Copy link
Contributor Author

seregazhuk commented Dec 28, 2017

@clue I've updated the request according to your comments :octocat:

clue
clue approved these changes Dec 28, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update! 👍

@WyriHaximus WyriHaximus merged commit f8f029c into reactphp:master Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsor jsor jsor approved these changes

@WyriHaximus WyriHaximus WyriHaximus approved these changes

@clue clue clue approved these changes

Assignees

No one assigned

Labels

easy pick help wanted maintenance

Projects

None yet

Milestone

v0.8.1

Development

Successfully merging this pull request may close these issues.

4 participants

@seregazhuk @jsor @WyriHaximus @clue