kernel_cmdline: remove cfg(test) for require_value_* methods
#1555
Conversation
This appears to have been accidentally added somewhere along the way, and I would like to use these in composefs-boot. Signed-off-by: John Eckersberg <[email protected]>
jeckersb
force-pushed
the
untest-require_value
branch
from
d7a9149 to
f83c655
Compare
jeckersb
changed the title
cfg(test) for require_value_* methods`cfg(test) for require_value_* methods
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the require_value_of and require_value_of_utf8 methods public by removing their #[cfg(test)] attributes. The change is correct and achieves its goal. My review includes suggestions to improve these newly-public functions by making their error handling more specific and enhancing their documentation. Since the functions are now part of the crate's public API, these improvements would increase robustness and usability for consumers.
| @@ -106,7 +106,6 @@ impl<'a> Cmdline<'a> { | |||
| /// Find the value of the kernel argument with the provided name, which must be present. | |||
| /// | |||
| /// Otherwise the same as [`Self::value_of`]. | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
By removing this attribute, require_value_of becomes part of the public API. While this is the goal of the PR, the function's implementation could be improved for public use. The error message when a value isn't found is ambiguous; it doesn't distinguish between a missing key and a key that exists but has no value (a switch). For a robust public API, providing distinct errors for these cases is a best practice.
I recommend refactoring the function to use self.find() and then checking for the presence of a value to provide more specific error messages. Additionally, the documentation should be updated with an # Errors section to clearly describe the error conditions, as recommended by Rust API guidelines.
| @@ -118,7 +117,6 @@ impl<'a> Cmdline<'a> { | |||
| /// Find the value of the kernel argument with the provided name, which must be present. | |||
| /// | |||
| /// Otherwise the same as [`Self::value_of`]. | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
Similar to require_value_of, by removing this attribute, require_value_of_utf8 is now exposed as a public API. It shares the same issue with an ambiguous error message when a value is not found. It would be beneficial to refactor it to provide a clearer error when a key is present but has no value, versus when the key is missing entirely. This will make the API more robust and easier to use for developers consuming this crate.
This appears to have been accidentally added somewhere along the way,
and I would like to use these in composefs-boot.
Signed-off-by: John Eckersberg [email protected]