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

Feature/php tidy #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

outrunthewolf
Copy link

  • I tried to tidy up the PHP a bit here in line with spec.
  • I'm still a little unsure as to why the key string or what I see as the event namespace was so hardcoded, i'm not an expert so perhaps you could clarify. I've changed the structure and readme in-line with how I believe it could work better.
  • Tabs to 4 spaces
  • Constants to class variables
  • Block comments
  • Emit arguments passed inline rather than stripped out of the args

It should in essence not change the way the class already works, just tidies it up. Let me know what you think...

@rase-
Copy link
Owner

rase- commented Apr 23, 2015

Thanks for the PR!

I like moving the constants and doc comments, but I'm not very fond of tab spacing or newline before {, for example.

Could you maybe suggest these changes in smaller pieces?

@Padam87
Copy link

Padam87 commented May 26, 2015

@rase- This would be awesome for your library. I know that following the PSR standards can be weird at first, but you will get the hang of it. 👍 for the PR.

protected $_flags = array();


public function __construct($redis = false, $opts = array()) {
Copy link

Choose a reason for hiding this comment

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

Missing NL and phpdoc

@Padam87
Copy link

Padam87 commented May 26, 2015

src/binary.php file names should use StudlyCase, therefore src/Binary.php.

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.

3 participants