Skip to content

Fixed a load of docblocks #244

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 1 commit into from Mar 13, 2015
Merged

Fixed a load of docblocks #244

merged 1 commit into from Mar 13, 2015

Conversation

GrahamCampbell
Copy link
Contributor

No description provided.

@@ -104,7 +104,6 @@ public function teams()
* @param string $type role in the repository
* @param string $sort sort by
* @param string $direction direction of sort, ask or desc

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this blank line and not, for example, in the diff above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other's are blank lines of comment. This is just a dodgie blank line with nothing on it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to send a pull request later to standardise the annotation separation.

I want to keep the PRs atomic.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should add the missing * instead IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof Should I fix all cases then? There's loads of them...

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I would fix this dodgie blank line in the expected way.

Then, fixing the separation of params and return values could be done in a separate PR to make it easier for maintainers to review it, or it could be included in the same PR to have everything fixed at once. I don't know what @pilot prefers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping @pilot. What would you like me to do here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@GrahamCampbell make all in one PR, just to fix all at once pls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do.

@GrahamCampbell GrahamCampbell changed the title Fixed a few docblocks Fixed a load of docblocks Feb 28, 2015
@GrahamCampbell
Copy link
Contributor Author

Ping @pilot, @stof. Done. :)

@@ -25,7 +25,8 @@
class Repo extends AbstractApi
{
/**
* Search repositories by keyword:
* Search repositories by keyword:.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove dot at the end for all places, looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is happening because you're miss using the short description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, a dot is required because this is a sentence.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GrahamCampbell: in this case, you just need to remove the colon before the period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I'll do a find and replace for all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@GrahamCampbell
Copy link
Contributor Author

Ping @pilot.

pilot added a commit that referenced this pull request Mar 13, 2015
Fixed a load of docblocks
@pilot pilot merged commit 2d33d76 into KnpLabs:master Mar 13, 2015
@GrahamCampbell GrahamCampbell deleted the doc branch March 13, 2015 21:49
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