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

[WIP] Code review #13

Closed
wants to merge 6 commits into from
Closed

[WIP] Code review #13

wants to merge 6 commits into from

Conversation

EmanueleMinotto
Copy link
Contributor

I'm doing some code reviews (mainly documentation), let me know if everything is ok or not. :)

To Do:

  • PHPDoc for src/App.php
  • Object Calisthenics for the classes directly under GianArb\Penny
  • Move FQCN (thanks @samsonasik)
  • Apply a quotes convention (I know, I know... I'm pedant)

@EmanueleMinotto
Copy link
Contributor Author

@gianarb let me know if there's something wrong in the documentation

@gianarb
Copy link
Contributor

gianarb commented Aug 25, 2015

It sounds perfect only on question src/App.php docs is work in progress? :)

@gianarb gianarb added this to the 0.1.2 milestone Aug 25, 2015
@EmanueleMinotto
Copy link
Contributor Author

yes, I'll update the to do list above

@samsonasik samsonasik mentioned this pull request Aug 25, 2015
*
* @param \FastRoute\Dispatcher $router Inner router (based on Nikic FastRouter).
*/
public function __construct(\FastRoute\Dispatcher $router)
Copy link
Member

Choose a reason for hiding this comment

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

fqcn can be moved to use statement

@gianarb
Copy link
Contributor

gianarb commented Aug 25, 2015

@EmanueleMinotto can you submit more PRs? :) step by step.. At the moment we are developing quickly and I prefer this flow 💃

Thanks

@EmanueleMinotto
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants