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

Code cleanup pass #198

Closed
defuse opened this issue Apr 4, 2016 · 16 comments
Closed

Code cleanup pass #198

defuse opened this issue Apr 4, 2016 · 16 comments

Comments

@defuse
Copy link
Owner

defuse commented Apr 4, 2016

Look over all the code, check for:

  • Unreachable code (e.g. functions that are never called).
  • Files without trailing new lines.
  • Tabs that should be spaces.
  • Consistent capitalization (e.g. some static methods are CamelCase some are whateverThisIsCalled)
  • Comments are correct (and all the methods that need documentation have doc comments).
@defuse defuse added the v2.0 label Apr 4, 2016
@defuse defuse added this to the v2.0 release milestone Apr 4, 2016
@defuse
Copy link
Owner Author

defuse commented Apr 4, 2016

Maybe look up that PSR-2 thing or whatever it is :)

@larowlan
Copy link
Contributor

larowlan commented Apr 4, 2016

If you want PSR-2 I'm happy to take this on

@larowlan
Copy link
Contributor

larowlan commented Apr 4, 2016

Or if you want something else, still happy to take it on

@defuse
Copy link
Owner Author

defuse commented Apr 4, 2016

@larowlan: We want PSR-2. If you're going to do it, do it on top of the branch in #197 since there are some big changes that are about to be merged there.

@larowlan
Copy link
Contributor

larowlan commented Apr 4, 2016

Will do
On 5 Apr 2016 6:44 am, "Taylor Hornby" notifications@github.com wrote:

@larowlan https://github.com/larowlan: We want PSR-2. If you're going
to do it, do it on top of the branch in #197
#197 since there are some
big changes that are about to be merged there.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#198 (comment)

@defuse
Copy link
Owner Author

defuse commented Apr 4, 2016

@larowlan Awesome, thanks!

@chriseskow
Copy link
Contributor

I'd recommend using PHP-CS-Fixer, which can automatically modify the code to adhere to PSR-2 standards (among other style changes). You can also add it to composer.json's require-dev and add a .php_cs file to configure it and make it easier to run consistently.

@larowlan
Copy link
Contributor

larowlan commented Apr 4, 2016

Great minds think alike ;)
On 5 Apr 2016 7:16 am, "Chris Eskow" notifications@github.com wrote:

I'd recommend using PHP-CS-Fixer
https://github.com/FriendsOfPHP/PHP-CS-Fixer, which can automatically
modify the code to adhere to PSR-2 standards (among other style changes).
You can also add it to composer.json's require-dev and add a .php_cs file
to configure it and make it easier to run consistently.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#198 (comment)

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

Fix the comments in File: #212 (comment)

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

Also make sure the right kind of exception is being thrown (see #216).

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

What's the standard for those /** doc comments? Is there a tool that can generate good documentation from them so that the "single source of truth" is the comments in the code and the documentation people see is generated?

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

Only 9 non-test non-exception files. I'll timebox 20 minutes on each one today.

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

$ ack -i 'TODO|FIXME|HACK|XXX' src/ test/
src/KeyOrPassword.php
57:            // TODO: Is reusing the same $salt between PBKDF2 and HKDF acceptable?

src/Crypto.php
20:     * @param string $key // TODO: this is wrong

test/unit/PasswordTest.php
27:    // TODO more tests (of the checksummed encoding, etc.)

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

Here's the "standard": https://en.wikipedia.org/wiki/PHPDoc

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2016

"The Long Description continues for as many lines as desired and may contain HTML markup for display formatting." loooool nope nope nope

@defuse
Copy link
Owner Author

defuse commented Apr 10, 2016

Done in #220.

@defuse defuse closed this as completed Apr 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants