Skip to content

Refactor #39

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

Merged
merged 7 commits into from
Nov 24, 2015
Merged

Refactor #39

merged 7 commits into from
Nov 24, 2015

Conversation

sagikazarmark
Copy link
Member

Things worth to mention:

  • Removed the package overview page. We should readd it at some point if it is necessary, but from the refactoring point of view I just couldn't place it anywhere.
  • Moved all HTTPlug related stuff under the HTTPlug group
  • Moved all other stuff under the Components group
  • Line character limits
  • Added some text to the index

Please review as I cannot read any more docs and I am sure it is full of mistakes.

@@ -33,7 +38,7 @@ class MyClass
protected $httpClient;

/**
* @param HttpClient|null $httpClient Client to do HTTP requests, if not set, autodiscovery will be used to find a HTTP client.
* @param HttpClient|null $httpClient
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the phpdoc explanation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it's an addition. The example code is pretty clean I think. I can add it back if you think it adds any value.

Copy link
Contributor

@dbu dbu Nov 24, 2015 via email

Choose a reason for hiding this comment

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

@dbu
Copy link
Contributor

dbu commented Nov 24, 2015

great job! i commented on quite some places (and github did not see some of the moves, considering whole files as new. so i probably reviewed some text that you did not change at all)

@sagikazarmark
Copy link
Member Author

Thanks for the review. I applied the suggested changes, commented on a few. We will probably have to proofread the whole docs before release.

@dbu
Copy link
Contributor

dbu commented Nov 24, 2015

great. i suggest we merge this and do further proofreading / review / improvements on the doc as people see things. this is a PR that should not stay open for long, or we risk annoying merge conflicts due to all the files moving around.

@sagikazarmark
Copy link
Member Author

Agree.

sagikazarmark added a commit that referenced this pull request Nov 24, 2015
@sagikazarmark sagikazarmark merged commit f76ccf6 into master Nov 24, 2015
@sagikazarmark sagikazarmark deleted the refactor branch November 24, 2015 09:55
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.

2 participants