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

Use backticks around TryFrom and FromStr in the output comments #757

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

proski
Copy link
Contributor

@proski proski commented Jan 28, 2025

Missing backticks trigger the pedantic doc_markdown lint in clippy.

Missing backticks trigger the pedantic `doc_markdown` lint in clippy.
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! How did you encounter this? Would it make sense to enable certain clippy lints in testing to ensure that we don't backslide?

@proski
Copy link
Contributor Author

proski commented Jan 28, 2025

I enabled all clippy categories apart from "nursery" in Cargo.toml:

[lints.clippy]
correctness = { level = "deny", priority = -1 }
suspicious = { level = "warn", priority = -1 }
complexity = { level = "warn", priority = -1 }
perf = { level = "warn", priority = -1 }
style = { level = "warn", priority = -1 }
pedantic = { level = "warn", priority = -1 }
cargo = { level = "warn", priority = -1 }

I was using import_types! originally. Clippy doesn't warn about results of macro expansion. Then I switched to build.rs as I wanted to have more control. Clippy warns about included files. Also, there is no way to suppress lints for macro expansion only, and I didn't want to suppress them for the whole file that deals with the loaded JSON file. So I added some regex replacements to build.rs.

All warnings are from clippy::doc_markdown (part of clippy::pedantic). One warning is about "TryFrom or FromStr". The other is for the first comment line above the struct definition, e.g. ///SenderConfig in this example:

///SenderConfig
///
/// <details><summary>JSON schema</summary>
///
/// ```json
///{
///  "title": "SenderConfig",

I was short on time when I made this PR, but now I see how to fix it. The comment comes from make_doc and there is a way to work it around by adding a description. But if there is no description, the name should be in backticks.

The test would be to run cargo clippy with all lints on the generated code.

On a related note, I'm surprised that the CI didn't run on my pull requests.

@ahl ahl merged commit 8444c21 into oxidecomputer:main Feb 1, 2025
4 checks passed
@proski proski deleted the clippy-pedantic branch February 3, 2025 05:12
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 this pull request may close these issues.

2 participants