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

Sharing: Add filter docblock for is_jetpack_site #2064

Merged
merged 2 commits into from
Jun 14, 2015

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Apr 30, 2015

As mentioned by @jeherve at #1946 (comment), new filter usage should include a DocBlock or DocBlock reference. This pull request adds a DocBlock definition for the is_jetpack_site filter usage in sharing-service.php. The filter is used elsewhere already, and other updates will need to be made.

General questions related to filter documenting:

  • How do we reconcile WordPress.com file syncing for referenced DocBlocks?
  • Should we prefer to document add_filter or apply_filter usage?
  • Is there a guideline on which filter to prefer the DocBlock be fully described (not referenced)?

@aduth aduth force-pushed the update/sharing-jetpack-site-filter branch from ad0c1ab to 2f7acda Compare April 30, 2015 18:59
@jeherve
Copy link
Member

jeherve commented Apr 30, 2015

How do we reconcile WordPress.com file syncing for referenced DocBlocks?

That's something we can handle with the merge script, I think.

Should we prefer to document add_filter or apply_filter usage?

apply_filters.
https://make.wordpress.org/core/handbook/inline-documentation-standards/php-documentation-standards/#4-hooks-actions-and-filters

Is there a guideline on which filter to prefer the DocBlock be fully described (not referenced)?

Preferably the place where it was first introduced, then follow core's way of referencing duplicate filters:
https://make.wordpress.org/core/handbook/inline-documentation-standards/php-documentation-standards/#4-1-duplicate-hooks

@jeherve jeherve added this to the 3.5.4 milestone May 9, 2015
@dereksmart dereksmart added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels May 13, 2015
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 9, 2015
dereksmart added a commit that referenced this pull request Jun 14, 2015
…filter

Sharing: Add filter docblock for `is_jetpack_site`
@dereksmart dereksmart merged commit b0b43b1 into master Jun 14, 2015
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 14, 2015
@dereksmart dereksmart deleted the update/sharing-jetpack-site-filter branch June 14, 2015 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs [Feature] Sharing Post sharing, sharing buttons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants