Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Minor documentation and code cleanup #894

Merged
merged 2 commits into from Apr 19, 2013

Conversation

Projects
None yet
4 participants
Contributor

mikegreiling commented Apr 18, 2013

Refactored the constructor methods and public properties for lithium\net\Message and its subclasses, fixed a few minor things, and added documentation.

@jails jails commented on the diff Apr 18, 2013

action/Request.php
@@ -144,27 +137,29 @@ class Request extends \lithium\net\http\Request {
protected $_locale = null;
/**
- * Adds config values to the public properties when a new object is created.
- * Pulling request data from superglobals if `globals` is set to `true`.
+ * Adds config values to the public properties when a new object is created, pulling
+ * request data from superglobals if `globals` is set to `true`.
*
* @param array $config Configuration options : default values are:
@blainesch

blainesch Apr 18, 2013

Member

Tabbing should only occur for properties of the docblocks.

Shouldn't this stay split? Short/long description.

@nateabele

nateabele Apr 18, 2013

Owner

@jails This looks fine as-is.

@mikegreiling

mikegreiling Apr 18, 2013

Contributor

I guess I didn't read the documentation. I was merely changing it to match the indentation I saw in some other comment blocks.

@mikegreiling

mikegreiling Apr 18, 2013

Contributor

When you say fine as-is, do you refer to the PR or the way it was before? Should I revert the changes in indentation?

Contributor

mikegreiling commented Apr 19, 2013

I'd like to update this to get it merged, but I'd like to get something clarified first regarding the documentation spec:

It says: "Each tag should have a description. If that description spans multiple lines the wrapped lines must be indented using 4 spaces."

Then gives the following example, which uses 8 spaces (to line up with @return) and another which uses 7 (to line up with @param)

[...]
 * @return array Nunc rutrum luctus velit, eu mattis quam aliquet quis. Morbi pulvinar 
 *         posuere neque, eget consectetur nisi tristique nec. Cras in tellus orci, 
 *         ac venenatis magna.
[...]

[...]
 * @param array $options This array may contain groups of settings used to override
 *        the default loader initialization for both the framework and the application.
 *        For details on allowed settings, see `Libraries::add()`. Valid options are:
 *        - `'lithium'`: Overrides base path and class load settings for core framework.
 *        - `'app'`: Overrides base path and class load settings for application.
[...]

Plus I also see a lot of docblocks where the wrapped lines match up with the end of the data type (i.e. after @return array or upwards of 14 spaces)...

[...]
 * @param array $options This array may contain groups of settings used to override
 *              the default loader initialization for both the framework and the application.
 *              For details on allowed settings, see `Libraries::add()`. Valid options are:
 *              - `'lithium'`: Overrides base path and class load settings for core framework.
 *              - `'app'`: Overrides base path and class load settings for application.
[...]

So before I go back and correct them, which is it?

Contributor

jails commented Apr 19, 2013

I think having >10 spaces just for formating is a lot of waste for a 100 max char/line. Personnaly I align on the type (i.e 7 or 8 spaces).

Contributor

mikegreiling commented Apr 19, 2013

Does this look better?

Contributor

jails commented Apr 19, 2013

yeah, that's perfect for me.

@nateabele nateabele and 1 other commented on an outdated diff Apr 19, 2013

action/Response.php
public function __construct(array $config = array()) {
- $defaults = array(
@nateabele

nateabele Apr 19, 2013

Owner

The $defaults variable is convention and should stay as-is.

@mikegreiling

mikegreiling Apr 19, 2013

Contributor

ok, I can change it back... I had seen that in a recent PR for \action\Request, $defaults = array( had been changed to $config += array( so I decided to update these the same way.

@nateabele

nateabele Apr 19, 2013

Owner

Oh, I missed that. If you happen to know where that is, do me a favor and switch that back as well.

The reason it's like that is because I've had a parser in the works since forever ago for li3_docs, which matches the array keys in $defaults to whatever's listed for $config or $options in the docblock. I should probably finish that...

Owner

nateabele commented Apr 19, 2013

Oh, and if you could squash your commits when you're done, that'd be awesome. Thanks again for all the contributions lately.

Contributor

mikegreiling commented Apr 19, 2013

I can squash them if you want me to, but I figured they were fairly unrelated commits...

Owner

nateabele commented Apr 19, 2013

Ah, you're totally right. Carry on. :-)

quality fixes and expansion of docblocks, minor code changes for cons…
…istency and readability.

also relocated constructor properties for `\net\http\Request` which had no business inside `\net\http\Message`.
Contributor

mikegreiling commented Apr 19, 2013

how does this look?

nateabele added a commit that referenced this pull request Apr 19, 2013

Merge pull request #894 from pixelcog/message-qa
Minor documentation and code cleanup

@nateabele nateabele merged commit c038b4e into UnionOfRAD:dev Apr 19, 2013

1 check passed

default The Travis build passed
Details

@mikegreiling mikegreiling deleted the pixelcog:message-qa branch Apr 20, 2013

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