search

microsoft / openvmm

Home of OpenVMM and OpenHCL.
http://openvmm.dev/
MIT License
1.36k stars 59 forks source link

Delete unwanted lints in future additions #146

Closed KGrewal1 closed 4 days ago

KGrewal1 commented 4 days ago

Start of what is needed to activate the unused self lint - did not touch public functions yet or incomplete functions (where this would have been largely meaningless). Turns many methods into associated functions

KGrewal1 commented 4 days ago

@microsoft-github-policy-service agree

daprilik commented 4 days ago

Hey! Thanks for taking an interest in OpenVMM.

I'm assuming this is a PR to resolve the TODO comment we currently have in our Cargo.toml lints table? i.e: https://github.com/microsoft/openvmm/blob/b9acac7/Cargo.toml#L598-L601. For context: that comment was added ~4 years ago, right around when we did initial clippy bringup in the project. A lot has changed since then, including our opinions on code style.

Unfortunately, this isn't a change we'd want to accept at this time.

We do appreciate the exploratory work you've put in here, and looking at the PR, we can clearly see that there are indeed certain cases where member functions should be refactored to be standalone helper functions (or even just constants). That said, there are also many cases where a method might take &self for more nuanced reasons, such as the method being partially complete, or in order to improve code readability by having complimentary methods use the same signature.

If you'd still like a contribution, I'd be happy to merge a PR which simply deleted those TODO comments in the Cargo.toml file!

Ultimately, we want to remove most of the lints in the # Possible future additions: block, and just leave doc_markdown (which I do think has merit).

# Possible future additions:
#
# useful, but _very_ tedious to fix
#   doc_markdown = "warn"
KGrewal1 commented 4 days ago

Slightly poor form, as opposed to opening a new PR, but have changed this PR to do that