Repository navigation
Conversation
|
Thanks for contributing! Sorry for letting you wait. Holidays and higher priority product work delayed the review of your PR. We'll take a look. |
uklotzde
left a comment
uklotzde
left a comment
There was a problem hiding this comment.
Only some minor remarks from my side.
| impl TranslateBrowsePathsToNodeIdsResponse { | ||
| #[must_use] | ||
| pub fn results(&self) -> Option<ua::Array<ua::BrowsePathResult>> { | ||
| ua::Array::from_raw_parts(self.0.resultsSize, self.0.results) |
There was a problem hiding this comment.
Please add the TODO comments about returning "non-owned" results from similar methods. Just for consistency.
There was a problem hiding this comment.
Added the TODO, same as the other responses.
| /// # Errors | ||
| /// This fails only when the entire request fails. When a path does not exist or cannot be | ||
| /// translated, an inner `Err` is returned. | ||
| pub async fn translate_browse_path( |
There was a problem hiding this comment.
This is just a convenience method for very simple use cases.
We should strive to reduce the amount of redundant code by calling self.translate_many_browse_paths(). Even if it has a minor performance overhead.
See also: read_attributes()/read_many_attributes()
There was a problem hiding this comment.
Done, it now calls translate_many_browse_paths() like read_attributes() does.
| return Err(Error::internal("unexpected number of browse path results")); | ||
| } | ||
|
|
||
| let targets: Vec<_> = results.iter().map(to_browse_path_result).collect(); |
There was a problem hiding this comment.
Let's use the turbofish variant here. See also: #312
There was a problem hiding this comment.
Switched to .collect::<Vec<_>>()
| } | ||
| } | ||
|
|
||
| // translate many browse paths |
There was a problem hiding this comment.
We use sentence-style line comments for consistency and readability. Starting with a capital letter and ending with a period. Even if the comment is not a complete sentence.
|
We also need a changelog entry. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1176032 to
5406564
Compare
- Add TODO about returning non-owned results, consistent with other responses. - Implement translate_browse_path() via translate_many_browse_paths(). - Use turbofish with collect(). - Use sentence-style line comments in example. - Stop checking hard-coded node IDs in example, they change on the demo server. - Add changelog entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I rebased onto main and also changed the example to stop checking hard-coded node IDs. The demo server changed the ID for CoffeeBeanLevel, so the example now just prints the IDs it gets back. |
2 participants
No description provided.