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

Convert all of our open source Rust repos to use our standard lints #35

Closed
21 tasks done
repi opened this issue Mar 6, 2021 · 7 comments
Closed
21 tasks done

Convert all of our open source Rust repos to use our standard lints #35

repi opened this issue Mar 6, 2021 · 7 comments

Comments

@repi
Copy link
Contributor

repi commented Mar 6, 2021

We now have a template for our defined standard Rust & Clippy lints that all of our Rust projects should use as default: EmbarkStudios/rust-ecosystem#59.

We'll need to add this template to all of our existing crates, which likely also includes fixing up some code in the the different repos but many of them already have had some of these lints enabled. So let's do PRs on all of our repos to copy over the lints, think we can wait a week or so though because may soon be a v0.3 of the lints (EmbarkStudios/rust-ecosystem#60), that we would have to copy over again. I've done a couple of the bigger ones already, and was pretty fast.

Is a good learning Rust experience to enable lints and fix up the code, so hope multiple can take on and follow up with this.

This is a tracking issue to add the lints to all of our open source Rust projects:

@XAMPPRocky
Copy link
Member

I don't know if we want to do this or not, but it occur to me that we could reduce the amount the code we'd have to change each time by pulling in the lints.rs file at build time. This would probably the most helpful for repos that are cargo workspaces, and have a lot of different crates that need to be updated each time.

// --- In `build.rs`
type Result<T> = std::result::Result<T, Box<dyn std::error::Error>>;
const URL: &str = "https://raw.githubusercontent.com/EmbarkStudios/rust-ecosystem/main/lints.rs";

fn from_network() -> Result<String> {
    Ok(reqwest::blocking::get(URL)?.text()?)
}

fn from_filesystem() -> Result<String> {
    Ok(std::fs::read_to_string("./lints.rs")?)
}

fn main() -> Result<()> {
    let out_dir = std::path::PathBuf::from(std::env::var("OUT_DIR")?);

    std::fs::write(out_dir.join("lints.rs"), from_network()?)?;

    Ok(())
}

// --- In `src/lib.rs`/`src/main.rs`
include!(concat!(env!("OUT_DIR"), "lints.rs"));

@Jake-Shadle
Copy link
Member

I don't think we would want to do that, even though the goal is nice, downloading a file at build time that is only useful for development is unkind to users.

@repi
Copy link
Contributor Author

repi commented Mar 8, 2021

Yeah no we can't do that, every lint upgrade has to be manual anyway to comply to the lints. So has to be copy'n'pasted to the crate roots and then fix the warnings that the new lints cause. But later when Cargo does have support for a separate lint file it will be easier to just copy over a file per repo at least EmbarkStudios/rust-ecosystem#22.

@repi
Copy link
Contributor Author

repi commented Mar 10, 2021

@XAMPPRocky @lpil v0.3 of our lints are defined now (EmbarkStudios/rust-ecosystem#60 ), so now would be a good time to start propagate the lint set to all of our crates and repos.

Nothing time critical but something we'll need to do

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Mar 11, 2021

I've gone ahead and created an issue in all the repositories that didn't already have an open PR and tagged the issues with help wanted and good first issue in case anyone in the community wants to take them on. Example

@repi Should I update the links in the top comment to point to the issues instead of just the repos?

@repi
Copy link
Contributor Author

repi commented Mar 11, 2021

@XAMPPRocky thx, and sure!

@XAMPPRocky
Copy link
Member

All of our crates now use v0.3

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

No branches or pull requests

3 participants