Conversation
…llowed panic states
|
Great job! No new security vulnerabilities introduced in this pull request |
|
The CI failure in Lint seems to also be failing on main. But when I run |
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
Not a codeowner, but changes look good from my perspective.
…ing-code-and-mutex-panics
Deploying contributing-docs with
|
| Latest commit: |
96174cd
|
| Status: | 🚫 Build failed. |
| was violated. This provides highly readable code and crash logs. For example: | ||
|
|
||
| ```rust | ||
| let some_object = self.shared_object.lock().expect("Mutex not to be poisoned."); |
There was a problem hiding this comment.
This does not follow the recommended pattern, https://doc.rust-lang.org/std/error/index.html#common-message-styles
There was a problem hiding this comment.
Could you specify which aspect does not fit the recommended pattern?
I believe it most closely matches the third one.
In the “expect as precondition” style, we would instead describe the reason we expect the Result should be Ok. With this style we would prefer to write:
let path = std::env::var("IMPORTANT_PATH")
.expect("env variableIMPORTANT_PATHshould be set bywrapper_script.sh");
docs/contributing/code-style/rust.md
Outdated
| If you still sense that explicit argument documentation would be helpful, this could be a hint to | ||
| extract the arguments into a separate struct to improve clarity and enforce type safety. | ||
|
|
||
| Let's say `sum_positive_integers()` is now a library function in our utils module. |
There was a problem hiding this comment.
This only applies to the good example. Skip?
There was a problem hiding this comment.
Ah, this is meant to be a reference to the function of the same name in the "Functions" section. I think if the "bad" example below is renamed to also be called sum_positive_integers, it would encapsulate the message for this section.
Perhaps having a "good" and "bad" section in "Functions" above (copying the sum() bad example up there) would help. So then we have a good and bad example for both sections.
…d-mutex-panics' of github.com:bitwarden/contributing-docs into neuronull/pm-31660/rust-style-guide-documenting-code-and-mutex-panics
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31660
📔 Objective
During code review on one of my PRs in clients repo, @quexten astutely flagged some discrepancies of my code to the style guide. While I adjusted some of my code, I did feel that some of the contributing docs' guidelines could use a little more color, given the nature of it covering all of our repositories, which includes the sdk-internal library, and desktop_native in clients.
Very open to discussion on the topics and adjustments to the examples etc.
📸 Screenshots