Document logHandler.getOnErrorSoundRequested() extension point#16017
Conversation
See test results for failed build of commit 5629cfc4b2 |
|
Cc @XLTechie in case you want to review this. |
|
Note that I have inserted the new paragraph following alphabetic order, even if only two paragraphs are not ordered originally. |
| ++ logHandler ++[logHandlerExtPts] | ||
|
|
||
| || Type | Extension Point | Description | | ||
| | ``Action`` | ``getOnErrorSoundRequested()`` | Triggered every time an error sound needs to be played. | |
There was a problem hiding this comment.
You are not documenting an extension point, but a function which returns, or creates and returns, an extension point. This is not how any other extension point is documented or used.
Either in the table description, or in a note immediately after the table, or both, this should be explained further.
A starting point may be:
| | ``Action`` | ``getOnErrorSoundRequested()`` | Triggered every time an error sound needs to be played. | | |
| | ``Action`` | ``getOnErrorSoundRequested()`` | Called to obtain the Action which is used to notify when an error sound should be played. | | |
| In order to avoid difficulties with early or circular importing, this extension point has been encapsulated by a function, and is silently created upon first use. | |
| Other than calling it as a function, it may be used like any other extension point. |
I admit that my memory of your work in that module is vague at best, so I may have misunderstood something about how it works. But that's what I got from reading the code a minute ago, and what I remember from the original PR. Hopefully that helps some.
There was a problem hiding this comment.
Thanks @XLTechie for your review.
I have made an updates in de8ec3e based on your review comment.
Since the dev doc documents extension points in this paragraph, I have documented the private extension point, but have indicated explicitely that you should use the function to retrieve it.
I have also added the rest of the documentation in the code as it is done for other extension points, since the dev doc is only a listing with a basic description of the extension points and it is indicated that more information can be found in the code.
| ++ logHandler ++[logHandlerExtPts] | ||
|
|
||
| || Type | Extension Point | Description | | ||
| | ``Action`` | ``_onErrorSoundRequested`` | Triggered every time an error sound needs to be played. This extension point should not be used directly but retrieved calling ``getOnErrorSoundRequested()`` instead. | |
There was a problem hiding this comment.
I'd suggest to avoid mentioning _onErrorSoundRequested explicitly both here and in the change log. The fact that this exists as a private variable is irrelevant to developers, and they should only access it via the getter function. By mentioning it were essentially committing to the private part of the API.
There was a problem hiding this comment.
I disagree - the dev guide and changes for developers is for core developers and other contributors, not just add-on developers. The distinction here on usage and API boundary is fairly clear.
|
hopefully retriggering build |
XLTechie
left a comment
There was a problem hiding this comment.
LGTM. Thanks @CyrilleB79!
| ++ logHandler ++[logHandlerExtPts] | ||
|
|
||
| || Type | Extension Point | Description | | ||
| | ``Action`` | ``_onErrorSoundRequested`` | Triggered every time an error sound needs to be played. This extension point should not be used directly but retrieved calling ``getOnErrorSoundRequested()`` instead. | |
There was a problem hiding this comment.
While initially I agreed with @lukaszgo1's thinking that we shouldn't document a private variable like this, I think the way it is being done here makes clear how it is expected to be used. Documenting the actual variable which holds the extPoint is closer to the spirit of the rest of the Dev Guide chapter. Anyone who misuses it after reading the clear description @CyrilleB79 has written, is taking their own stupid risks.
…ccess#16017) Fix-up of nvaccess#15691. Summary of the issue: In nvaccess#15691, a new extension point has been created. Unfortunately I have forgotten to document it. Description of user facing changes Document this extension point in the dev guide as well as in the change log.
Link to issue number:
Fix-up of #15691.
Summary of the issue:
In #15691, a new extension point has been created. Unfortunately I have forgotten to document it.
Description of user facing changes
Document this extension point in the dev guide as well as in the change log.
Description of development approach
N/A
Testing strategy:
N/A
Known issues with pull request:
None
Code Review Checklist: