Code style improvements + optional CI job (clippy, rustfmt, rustdoc)

[phip1611]

• add clippy check as required
• add rustfmt check as required
• add rustdoc check as required (#![deny(rustdoc::all)] works since stable 1.52.0)

To improve and enforce code quality.

I'm not sure if we still use Travis CI.

What's your opinion on this @Caduser2020 @IsaacWoods ?

[phip1611]

Tag is private, therefore rustdoc failure

[IsaacWoods]

Hmm, I've tried having these sorts of checks in CI with communal projects before, and I think I'm generally against it. It makes merging every PR, especially from new contributors, a bit of an uphill battle that doesn't leave a good impression for potential contributors. I think with a relatively small project, doing a clippy PR every so often, as well as manually asking for really bad formatting to be run through rustfmt before you merge is good enough. Open to alternative views though.

I'm not sure if we still use Travis CI.

Ah no, my mistake. Afaik, Travis CI is basically dead (it stopped reporting status updates for this repo ages ago). I fixed that by moving us oven to Actions, but must've forgotten to delete the Travis files. Feel free to remove them :)

[phip1611]

@IsaacWoods

It makes merging every PR, especially from new contributors, a bit of an uphill battle that doesn't leave a good impression for potential contributors

Hm. On the one hand I can understand this. On the otherhand, I'm certain that contributors of Multboot2, which is not a super basic domain but deep into OS development, will also not be total Rust newbees. Furthermore, it's a good opportunity for them to learn to use rust fmt and rust clippy.

Any other opinion? @Caduser2020 @phil-opp

[phip1611]

Any update here @Caduser2020 @phil-opp @IsaacWoods ?

[IsaacWoods]

I stand by my view that this has more disadvantages than advantages, and that CI failures for slight differences in code style are generally unhelpful (this is especially bad in OSdev when lots of people are a few nightlies behind, and so rustfmt complains not because you didn't format, but you formatted with the wrong version of it). Lots of people's editors autoformat, and I'd rather ask for an individual PR to be reformatted if I felt that the style was confusing or egregious. I also think that it generally annoys people, newbees and otherwise, and most people become exposed to those tools anyway.

So my vote is to close, but I'm also not going to make this a hill to die on, so if you feel particularly strongly, you can merge.

[phip1611]

I stand by my view that this has more disadvantages than advantages, and that CI failures for slight differences in code style are generally unhelpful (this is especially bad in OSdev when lots of people are a few nightlies behind, and so rustfmt complains not because you didn't format, but you formatted with the wrong version of it

@IsaacWoods

Theoretically, we could use a "rust-toolchain.toml" to prevent that :)

But I do not insist on the CI requirement. I like it for private projects but will remove it here. Will keep the formatting and style improvements, tho. After that I can finally start with the multiboot2-header crate.. took a little bit longer :)

[phil-opp]

Any other opinion? @phil-opp

First of all, I'm not maintaining the multiboot2 crate anymore, so my opinion on this doesn't really matter :D.

I don't think that it's a good idea to do add #![deny(clippy::all)] or #![deny(rustdoc::all)] to the code. The problem is that new clippy/rustdoc versions sometimes introduce new warnings, which would then break everyone who depends on this library. It's better to pass the deny argument on the command line in the CI script.

My personal preference is to keep warnings as warnings and use e.g. https://github.com/actions-rs/cargo to show the warnings in the "Files Changed" tab on GitHub. This way, the maintainers can decide whether the warning should be fixed or not, and it doesn't affect the mergeability of newcomer PRs.

Enforcing rustfmt is ok in my opinion, but I agree with @IsaacWoods that doing it on a crate that targets both stable and nightly can lead to problems. If you decide to add such a check, I would again recommend to create a separate CI job for it, which is not required to pass. This way, formatting problems don't block merging of newcomer PRs (they should still be fixed in a follow-up PR though).

[phip1611]

I would again recommend to create a separate CI job for it, which is not required to pass. This way, formatting problems don't block merging of newcomer PRs (they should still be fixed in a follow-up PR though).

I agree, let's do it that way :)

[ghost]

I agree with @IsaacWoods for this. My build of rust is often a few days behind and rustfmt would become out of sync. I am okay with fixes for these warnings, but breaking CI is not worth it. Vote to close.

[phip1611]

After all, this commit/PR

• introduces sensible style checks as optional CI task,
• removes Travis CI, and
• applies a few code style fixes.

The new CI job should be optional by default, because I think one has to manually mark them as required for PRs.