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

Avoid undocumented panics and enable clippy::missing-panics-doc #140

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented 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.
GUS-W-10167912.

@edmorley edmorley self-assigned this Nov 11, 2021
@edmorley
Copy link
Member Author

(Though I still feel that perhaps read_from_layer_dir() / layer_path_specs could do with a types/approach refactor to avoid this unreachable branch in the first place, but not worth it for now)

@edmorley edmorley requested a review from Malax November 11, 2021 11:09
@edmorley edmorley changed the title Avoid undocumented panics and enable clippy::missing-panics-doc Avoid undocumented panics and enable clippy::missing-panics-doc Nov 11, 2021
libcnb/src/layer_env.rs Outdated Show resolved Hide resolved
@edmorley edmorley force-pushed the edmorley/missing-panics-docwarning branch from 62b707c to 5ce9928 Compare November 11, 2021 13:49
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 edmorley force-pushed the edmorley/missing-panics-docwarning branch from 5ce9928 to 9098757 Compare November 11, 2021 13:51
@edmorley edmorley merged commit f09efdb into main Nov 11, 2021
@edmorley edmorley deleted the edmorley/missing-panics-docwarning branch November 11, 2021 14:02
@edmorley edmorley added this to the 0.4.0 milestone Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix clippy::missing-panics-doc warning
2 participants