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

cleanup #5

Merged
merged 4 commits into from
May 6, 2014
Merged

cleanup #5

merged 4 commits into from
May 6, 2014

Conversation

cordoval
Copy link
Contributor

@cordoval cordoval commented May 6, 2014

Sent with Gush

@cordoval cordoval changed the title cleanup [WIP] cleanup May 6, 2014
@cordoval cordoval changed the title [WIP] cleanup cleanup May 6, 2014
@cordoval
Copy link
Contributor Author

cordoval commented May 6, 2014

@nVitius i don't know why are we doing options or arguments with upper cases, i know it will probably make you change code but if we can do this in a way that you can do it then let's switch, if not then no worries.

@nVitius
Copy link

nVitius commented May 6, 2014

@cordoval In AWS' API documentation they are written in uppercase like that, so I did the same for mine. If other people think they should be lowercase too though, I can look into changing it.
@shideon Thoughts?

@cordoval
Copy link
Contributor Author

cordoval commented May 6, 2014

no need to spend time on that, let's just leave them at that. Just thought to mention that arguments and options are preferrably lower cases.

@cryptiklemur
Copy link

Typically, they should be lowercase, but that is a good question

nVitius pushed a commit that referenced this pull request May 6, 2014
@nVitius nVitius merged commit 3a2624b into uecode:master May 6, 2014
@johnpancoast
Copy link

@cordoval shouldn't we leave the @abstract in the class docblock?

@cryptiklemur
Copy link

@shideon The @abstract tag is only valid in PHP 4, PHP 5 has a keyword abstract.

I thought so too, but, apparently not

@cordoval
Copy link
Contributor Author

cordoval commented May 6, 2014

I tend not to be repetitive unnecessarily and often prefer to sacrify standards for clean code. I often get a @fabpot like summon when i add things that don't bring value: "does not add value." That is the main reason. It duplicates information or does not keep it DRY.

@cordoval cordoval deleted the 5-cleanup branch May 6, 2014 23:08
@cordoval
Copy link
Contributor Author

cordoval commented May 6, 2014

and now Aaron just gave me another reason to back it up 👶

@johnpancoast
Copy link

This does add value. There's code and there are API docs which the docblocks are for. We also have access on class members and methods but we still document them for the API docs.

/**
 * Do something
 *
 * @access protected
 */
protected function foo()
{
.....

@cordoval
Copy link
Contributor Author

cordoval commented May 6, 2014

oh no please

@cryptiklemur
Copy link

@shideon i'm not really sure that would add value either @shideon. When api docs are generated, they are generated with their access regardless of specifying @access.

@johnpancoast
Copy link

Does PHP Doc generate based on code or docblocks? I'm pretty sure it doesn't read your code (if it did, why have docblocks to begin with?)

@johnpancoast
Copy link

Could totally be wrong.

@cryptiklemur
Copy link

Not to mention, I would argue that Private and Protected functions don't really need to be documented in API, as they can't be called outside of the class they reside in.

I believe it does read some things from the code, like access.

@cryptiklemur
Copy link

er, at least private. Protected could be

@johnpancoast
Copy link

That doesn't matter. The api docs aren't just for clients who have only public access. It's also for others that do have that access.

@cryptiklemur
Copy link

If you can't call a private method, or override it, why does it matter?

@johnpancoast
Copy link

Because perhaps I'm a developer working on an extensive library and I do have that access but I want to view docs at a high level to understand things.

@nVitius
Copy link

nVitius commented May 6, 2014

If we're talking about docblocks on protected functions, I think it makes sense to have them. Sure, it doesn't matter when you're talking about API documentation, but it gives an insight into what the function does to whoever is working on the code.

@cryptiklemur
Copy link

I would agree to that. But i will still say that having @access tags are pointless. And in terms of API Documentation generation, I would say that private doesn't really need to be in the API Documentation.

@nVitius
Copy link

nVitius commented May 6, 2014

For the sake of consistency, if anything, I would probably leave them in there.

@cryptiklemur
Copy link

What consistency?

@nVitius
Copy link

nVitius commented May 6, 2014

http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.access.pkg.html

@access private prevents documentation of the following element (if enabled).

@johnpancoast
Copy link

Hmm, on the new docs page here: http://phpdoc.org/docs/latest/index.html

The @access tag is not there. So perhaps phpdoc does do a little reflection (or some equivalent form of reading) to read the access. Let's hope. That's an important feature from an API doc perspective (even if it's private and I'm editing or working with code that can call it).

@johnpancoast
Copy link

That's v2 of phpdoc.

@cryptiklemur
Copy link

@shideon if the code is documented properly, you shouldn't even need to know that its calling a private method

@johnpancoast
Copy link

I suppose it's API docs, so for that argument, you guy's are right. But if I'm editing the classes, it's different.

@cryptiklemur
Copy link

And again, i'm not saying there shouldnt be documentation for methods inside the classes. There definitely should be. even if @cordoval disagrees lol

@johnpancoast
Copy link

So about those lower case args and options... I agree. =P

@chrisamoore
Copy link

Quite the discussion.

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.

5 participants