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

Implement issue finder for lint names #5049

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clippy_dev/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ regex = "1"
lazy_static = "1.0"
shell-escape = "0.1"
walkdir = "2"
reqwest = { version = "0.10", features = ["blocking", "json"], optional = true }
serde = { version = "1.0", features = ["derive"], optional = true }

[features]
deny-warnings = []
issues = ["reqwest", "serde"]
154 changes: 154 additions & 0 deletions clippy_dev/src/issues_for_lint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
use crate::gather_all;
use lazy_static::lazy_static;
use regex::Regex;
use reqwest::{
blocking::{Client, Response},
header,
};
use serde::Deserialize;
use std::env;

lazy_static! {
static ref NEXT_PAGE_RE: Regex = Regex::new(r#"<(?P<link>[^;]+)>;\srel="next""#).unwrap();
}

#[derive(Debug, Deserialize)]
struct Issue {
title: String,
number: u32,
body: String,
pull_request: Option<PR>,
}

#[derive(Debug, Deserialize)]
struct PR {}

enum Error {
Reqwest(reqwest::Error),
Env(std::env::VarError),
Http(header::InvalidHeaderValue),
}

impl From<reqwest::Error> for Error {
fn from(err: reqwest::Error) -> Self {
Self::Reqwest(err)
}
}

impl From<std::env::VarError> for Error {
fn from(err: std::env::VarError) -> Self {
Self::Env(err)
}
}

impl From<header::InvalidHeaderValue> for Error {
fn from(err: header::InvalidHeaderValue) -> Self {
Self::Http(err)
}
}

impl std::fmt::Display for Error {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Self::Reqwest(err) => write!(fmt, "reqwest: {}", err),
Self::Env(err) => write!(fmt, "env: {}", err),
Self::Http(err) => write!(fmt, "http: {}", err),
}
}
}

pub fn run(name: &str, filter: &[u32]) {
match open_issues() {
Ok(issues) => {
for (i, issue) in filter_issues(&issues, name, filter).enumerate() {
if i == 0 {
println!("### `{}`\n", name);
}
println!("- [ ] #{} ({})", issue.number, issue.title)
}
},
Err(err) => eprintln!("{}", err),
}
}

pub fn run_all(filter: &[u32]) {
match open_issues() {
Ok(issues) => {
let mut lint_names = gather_all().map(|lint| lint.name).collect::<Vec<_>>();
lint_names.sort();
for name in lint_names {
let mut print_empty_line = false;
for (i, issue) in filter_issues(&issues, &name, filter).enumerate() {
if i == 0 {
println!("### `{}`\n", name);
print_empty_line = true;
}
println!("- [ ] #{} ({})", issue.number, issue.title)
}
if print_empty_line {
println!();
}
}
},
Err(err) => eprintln!("{}", err),
}
}

fn open_issues() -> Result<Vec<Issue>, Error> {
let github_token = env::var("GITHUB_TOKEN")?;

let mut headers = header::HeaderMap::new();
headers.insert(
header::AUTHORIZATION,
header::HeaderValue::from_str(&format!("token {}", github_token))?,
);
headers.insert(header::USER_AGENT, header::HeaderValue::from_static("ghost"));
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to give the user agent a slightly more descriptive name such as 'rust-clippy issue finder'

let client = Client::builder().default_headers(headers).build()?;

let issues_base = "https://api.github.com/repos/rust-lang/rust-clippy/issues";

let mut issues = vec![];
let mut response = client
.get(issues_base)
.query(&[("per_page", "100"), ("state", "open"), ("direction", "asc")])
.send()?;
while let Some(link) = next_link(&response) {
issues.extend(
response
.json::<Vec<Issue>>()?
.into_iter()
.filter(|i| i.pull_request.is_none()),
);
response = client.get(&link).send()?;
}

Ok(issues)
}

fn filter_issues<'a>(issues: &'a [Issue], name: &str, filter: &'a [u32]) -> impl Iterator<Item = &'a Issue> {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth adding some unit tests for this function

let name = name.to_lowercase();
let separated_name = name.chars().map(|c| if c == '_' { ' ' } else { c }).collect::<String>();
let dash_separated_name = name.chars().map(|c| if c == '_' { '-' } else { c }).collect::<String>();

issues.iter().filter(move |i| {
let title = i.title.to_lowercase();
let body = i.body.to_lowercase();
!filter.contains(&i.number)
&& (title.contains(&name)
|| title.contains(&separated_name)
|| title.contains(&dash_separated_name)
|| body.contains(&name)
|| body.contains(&separated_name)
|| body.contains(&dash_separated_name))
})
Copy link

@apiraino apiraino Jan 16, 2020

Choose a reason for hiding this comment

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

how about also checking the issue labels? If at some point in the future issues are then properly labeled with a lint-*, that could make easier to sort them

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, we would then need over 300 labels for all of the lints and the issues had to be labeled correctly. Labeling issues by hand wouldn't be feasible. Maybe we could write an automation for such a labeling. But as long, as we don't have such a system in place, I don't think adding this to this tool makes much sense.

But The lint-*/L-* label idea is definitely interesting! Maybe we could add the labels of the issues in the output of this tool 🤔

Choose a reason for hiding this comment

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

Maybe we could add the labels of the issues in the output of this tool

that's a great idea imo 👍

As a positive side effect, people can easily start using the right label when reporting, decreasing in time the amount of "unlabeled" linting issues.

Choose a reason for hiding this comment

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

Also, but I'm not sure it makes sense, as a starting point, I'd force feed the issueing ticketing system with all the labels with a quick cURL, so people can be instructed to use them immediately and we truncate immediately the input of "unlabeled" linting issues

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is, that "normal" users can't label their issue by them self, but Collaborators/Members have to do this. There is also @rustbot, but we can't check this automatically. We'd could write a GitHub Actions Workflow, that scans a new issue for a lint name and labels it accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

You can have issue templates that have a default label. I have some examples here: https://github.com/jyn514/rcc/tree/master/.github/ISSUE_TEMPLATE

Copy link

@apiraino apiraino Feb 17, 2020

Choose a reason for hiding this comment

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

Unless I'm wrong, for issue templates to be useful we need to have the user be able to automatically apply one label out of dozens hundreds and that would require having dozens hundreds of templates, therefore a scary list of templates when the user creates the issue :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're soon completely migrated to GHA, we could implement the labeler workflow for issues and PRs, that would do that. But the question, if we want to have hundreds of labels still stands.

Copy link
Member

@phansch phansch Feb 21, 2020

Choose a reason for hiding this comment

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

If we can automate the label creation, I would not be against it personally but that's probably for another PR

Choose a reason for hiding this comment

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

I agree. So, I'd add a check for the issue label here (because imo it's a sensible default) and then we find a way to manage the issue creation workflow.

}

fn next_link(response: &Response) -> Option<String> {
if let Some(links) = response.headers().get("Link").and_then(|l| l.to_str().ok()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some(links) = response.headers().get("Link").and_then(|l| l.to_str().ok()) {
if let Some(links) = response.headers().get(reqwest::headers::LINK).and_then(|l| l.to_str().ok()) {

if let Some(cap) = NEXT_PAGE_RE.captures_iter(links).next() {
Copy link
Member

Choose a reason for hiding this comment

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

Huh, is there no way to do this without a regex?

return Some(cap["link"].to_string());
}
}

None
}
55 changes: 51 additions & 4 deletions clippy_dev/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
#![cfg_attr(feature = "deny-warnings", deny(warnings))]

use clap::{App, Arg, SubCommand};
use clap::{App, Arg, ArgMatches, SubCommand};
use clippy_dev::*;
use std::path::Path;

mod fmt;
#[cfg(feature = "issues")]
mod issues_for_lint;
mod new_lint;
mod stderr_length_check;

Expand All @@ -15,7 +17,7 @@ enum UpdateMode {
}

fn main() {
let matches = App::new("Clippy developer tooling")
let mut app = App::new("Clippy developer tooling")
.subcommand(
SubCommand::with_name("fmt")
.about("Run rustfmt on all projects and tests")
Expand Down Expand Up @@ -98,8 +100,31 @@ fn main() {
Arg::with_name("limit-stderr-length")
.long("limit-stderr-length")
.help("Ensures that stderr files do not grow longer than a certain amount of lines."),
)
.get_matches();
);
if cfg!(feature = "issues") {
app = app.subcommand(
SubCommand::with_name("issues_for_lint")
.about(
"Prints all issues where the specified lint is mentioned either in the title or in the description",
)
.arg(
Arg::with_name("name")
.short("n")
.long("name")
.help("The name of the lint")
.takes_value(true)
.required_unless("all"),
)
.arg(Arg::with_name("all").long("all").help("Create a list for all lints"))
.arg(
Arg::with_name("filter")
.long("filter")
.takes_value(true)
.help("Comma separated list of issue numbers, that should be filtered out"),
),
);
}
let matches = app.get_matches();

if matches.is_present("limit-stderr-length") {
stderr_length_check::check();
Expand Down Expand Up @@ -128,10 +153,32 @@ fn main() {
Err(e) => eprintln!("Unable to create lint: {}", e),
}
},
("issues_for_lint", Some(matches)) => issues_for_lint(matches),
_ => {},
}
}

fn issues_for_lint(_matches: &ArgMatches<'_>) {
#[cfg(feature = "issues")]
{
let filter = if let Some(filter) = _matches.value_of("filter") {
let mut issue_nbs = vec![];
for nb in filter.split(',') {
issue_nbs.push(nb.trim().parse::<u32>().expect("only numbers are allowed as filter"));
}
issue_nbs
} else {
vec![]
};
if _matches.is_present("all") {
issues_for_lint::run_all(&filter);
} else {
let name = _matches.value_of("name").expect("checked by clap");
issues_for_lint::run(&name, &filter);
}
}
}

fn print_lints() {
let lint_list = gather_all();
let usable_lints: Vec<Lint> = Lint::usable_lints(lint_list).collect();
Expand Down