-
Notifications
You must be signed in to change notification settings - Fork 156
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
Clarify references to filters/hooks in README.md #462
Conversation
Fixed issue with three references in the "Caveats" heading which referenced code simply by saying "here" pointing to a line number which was now out of date. Added the filter/hooks by name to the documentation to make it easier for the reader to find the intended portion of the code, and updated the links to the current line numbers. Also added note of caution regarding the differing uses of the `dt_push_post` hook based on internal/external connections.
@dmchale Thanks for this PR! This looks good to me. I like linking directly to the line of code that has the filter but yeah, that's not super helpful if the documentation doesn't stay up-to-date. I think this is good for now and we'll just need to do a better job of reviewing this documentation as code changes happen. |
README.md
Outdated
@@ -76,9 +76,9 @@ To help inform our roadmap, keep adopters apprised of major updates and changes | |||
|
|||
## Known Caveats/Issues | |||
|
|||
__Remote Request Timeouts__ - With external connections, HTTP requests are sent back and forth - creating posts, transfering images, syncing post updates, etc. In certain situations, mostly commonly when distributing posts with a large number of images (or very large file sizes), using poorly configured or saturated servers / hosts, or using plugins that add significant weight to post creation, Distributor requests can fail. Although we do some error handling, there are certain cases in which post distribution can fail silently. If distribution requests are taking a long time to load and/or failing, consider adjusting the timeout; you can filter the timeout for pushing external posts [here](https://github.com/10up/distributor/blob/master/includes/classes/ExternalConnections/WordPressExternalConnection.php#L487). More advanced handling of large content requests, and improved error handling is on the road map for a future update. | |||
__Remote Request Timeouts__ - With external connections, HTTP requests are sent back and forth - creating posts, transfering images, syncing post updates, etc. In certain situations, mostly commonly when distributing posts with a large number of images (or very large file sizes), using poorly configured or saturated servers / hosts, or using plugins that add significant weight to post creation, Distributor requests can fail. Although we do some error handling, there are certain cases in which post distribution can fail silently. If distribution requests are taking a long time to load and/or failing, consider adjusting the timeout; you can filter the timeout for pushing external posts using the `dt_push_post_timeout` filter [here](https://github.com/10up/distributor/blob/master/includes/classes/ExternalConnections/WordPressExternalConnection.php#L620). More advanced handling of large content requests, and improved error handling is on the road map for a future update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can use links with commit hash, the content of those lines will be always relevant. For example: https://github.com/10up/distributor/blob/f7b60740e679bce4671ccd69a670abadce4f2f93/includes/classes/ExternalConnections/WordPressExternalConnection.php#L620
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only issue with the commit hash is that the remaining code may not be the current released code. Maybe we link to https://10up.github.io/distributor/dt_push_post_timeout.html instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do so, we need to keep the docs up to date as well. The current filter you linked above is out of date 😅: https://10up.github.io/distributor/includes_classes_ExternalConnections_WordPressExternalConnection.php.html#line605
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any task to regenerate the docs on release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dinhtungdu looks like they get regenerated on push to develop
: https://github.com/10up/distributor/blob/develop/.github/workflows/build-docs.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build docs actions are failing: https://github.com/10up/distributor/actions <- That's why it's out of date on the docs. But I agree that the best solution here is linking to the docs site. We just need to fix that action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I'm looking into refreshing the GitHub secret used by that GitHub Action to see if that helps resolve the Build Hook Docs action's errors. Assuming that works, I think we should link into those docs for this filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and changed the links to the docs site, we'll work to resolve the failing GitHub Action separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks @dmchale!
cheers @jeffpaul , and thanks to everyone who spent their time looking at / working on this :) |
Fixed issue #461 with three references in the "Caveats" heading which referenced code simply by saying "here" pointing to a line number which was now out of date. Added the filter/hooks by name to the documentation to make it easier for the reader to find the intended portion of the code, and updated the links to the current line numbers. Also added note of caution regarding the differing uses of the
dt_push_post
hook based on internal/external connections.Description of the Change
Fixed links, added names of filter & hooks. Added note regarding different use of same-named hook.
Alternate Designs
Alternatively, links could simply point to the files to prevent links to line numbers from becoming stale again in the future. As long as the name of the filter/hook was in the documentation, the reader could find it themselves within the file.
Benefits
The current links are not helpful. Improving the information provided to the reader provides value to having the links there at all.
Possible Drawbacks
None thought of
Verification Process
Checklist:
Applicable Issues
#461