Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clippy::missing-panics-doc warning #54

Closed
edmorley opened this issue Sep 8, 2021 · 1 comment · Fixed by #140
Closed

Fix clippy::missing-panics-doc warning #54

edmorley opened this issue Sep 8, 2021 · 1 comment · Fixed by #140
Assignees

Comments

@edmorley
Copy link
Member

edmorley commented Sep 8, 2021

I ran Clippy's pedantic lints (which are disabled by default since they are typically subjective or have high false positive rates) against this repository out of curiosity, and am filing a few issues for some of the lints that warned that seemed relevant (in part to give some easy issues for people to work through to get involved with this library).

Feel free to close this out if not wanted :-)

See:
https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc
https://rust-lang.github.io/api-guidelines/documentation.html#function-docs-include-error-panic-and-safety-considerations-c-failure

$ cargo clippy --all-targets --all-features -- -W clippy::missing-panics-doc
    Checking libcnb v0.2.0 (/Users/emorley/src/libcnb.rs)
warning: docs for function which may panic missing `# Panics` section
   --> src/layer_env.rs:242:5
    |
242 | /     pub fn read_from_layer_dir(layer_dir: impl AsRef<Path>) -> Result<LayerEnv, std::io::Error> {
243 | |         let mut result_layer_env = LayerEnv::new();
244 | |
245 | |         let bin_path = layer_dir.as_ref().join("bin");
...   |
288 | |         Ok(result_layer_env)
289 | |     }
    | |_____^
    |
    = note: requested on the command line with `-W clippy::missing-panics-doc`
note: first possible panic found here
   --> src/layer_env.rs:265:26
    |
265 |                     _ => panic!("Unexpected TargetLifecycle in read_from_layer_dir implementation. This is a libcnb implementation error!"),
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc
    = note: this warning originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)
@edmorley
Copy link
Member Author

Taking this one since Lillian didn't want it :-)

@edmorley edmorley self-assigned this Nov 11, 2021
edmorley added a commit that referenced this issue Nov 11, 2021
Switches the `panic!` in `LayerEnv::read_from_layer_dir()` to `unreachable!`, since:
- it more clearly indicates the intent (unreachable code unless there is a bug)
- it means we can globally enable the `clippy::missing-panics-doc` lint, without
  having to either:
  (a) add an unnecessary `# Panic` section to the docs (which would only add
  noise, given the caller needn't be aware of panics for internal bugs)
  (b) add an `#[allow(...)]` annotation to all of `read_from_layer_dir`, which could
  mean future new undocumented panics aren't caught by clippy.

Fixes #54.
edmorley added a commit that referenced this issue Nov 11, 2021
Switches the `panic!` in `LayerEnv::read_from_layer_dir()` to `unreachable!`, since:
- it more clearly indicates the intent (unreachable code unless there is a bug)
- it means we can globally enable the `clippy::missing-panics-doc` lint, without
  having to either:
  (a) add an unnecessary `# Panic` section to the docs (which would only add
  noise, given the caller needn't be aware of panics for internal bugs)
  (b) add an `#[allow(...)]` annotation to all of `read_from_layer_dir()`, which could
  mean future new undocumented panics aren't caught by clippy.

Fixes #54.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants