Closed jamestiotio closed 2 years ago
@jamestiotio Thanks for your efforts. Like @Rayman said the changes in the documentation is very welcome.
I think any breaking change is not accepted. As I don't plan any major version bump any time soon (and not so soon as well).
Breaking public API is for me a no-no.
Understood. In that case, I have reverted the breaking changes. Essentially, I have aligned the docstrings to simply follow the implementation as the source of truth.
To avoid confusion, I used the @also
tag for the getNodeDetails
method in core/Ros.js
to specify the two different possible callback function signatures. Do let me know if there is a better way to handle this.
Please cherry-pick https://github.com/RobotWebTools/roslibjs/commit/605ef08101e47d1cc0c20ca43a5c1d6f2cd2735f in your branch. Than we are good to go.
Thanks for your efforts.
@MatthijsBurgh Commit has been cherry-picked.
Public API Changes The refactoring to
core/Ros.js
might break some applications. However, the proposed changes would make the public API more straightforward in two ways:getNodeDetails
method, the callback function signature now remains the same, whether there is afailedCallback
function defined or not. Previously, the function signature differs between the two, which might lead to further confusion.Description This PR standardizes and improves the docstring, along with a few other small things.
More specifically, this PR does the following (in rough order of significance):
-s
suffix.Additionally, there are some non-docstring changes (again, in rough order of significance):
core/Ros.js
to align the callback parameters with the ones specified in the corresponding docstrings. Tests are also updated to follow the new callback function signatures.setSucceeded
,setAborted
, andsendFeedback
inactionlib/SimpleActionServer.js
with the respective docstrings.actionlib/ActionListener.js
(the optionstimeout
,omitFeedback
,omitStatus
, andomitResult
are not used there).actionlib/SimpleActionServer.js
andcore/SocketAdapter.js
.Relevant GitHub issues: This PR closes #542.