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

Lint for denylisted-strings, similar to denylisted-names #1624

Open
clarfonthey opened this issue Mar 15, 2017 · 7 comments
Open

Lint for denylisted-strings, similar to denylisted-names #1624

clarfonthey opened this issue Mar 15, 2017 · 7 comments
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Mar 15, 2017

This would search for specific string literals that are blacklisted, for example "foo", "bar", or "123".

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2017

Do you have examples of string literals to block? Do you mean "foo" and similar?

@clarfonthey
Copy link
Contributor Author

Currently, just the ones from the existing lint: foo, bar, etc. Could also add "123."

@shssoichiro
Copy link
Contributor

If this is added, I wouldn't want it to be turned on by default. I use "foo" and "bar" all over the place in my tests, and I do run clippy on my test code.

@clarfonthey
Copy link
Contributor Author

I mean, it already applies for variable names, this would just apply it for strings too. The idea is that your tests should use strings other than the most common ones.

@shssoichiro
Copy link
Contributor

Let's say I have a test for a JSON API that looks like this:

#[test]
fn create_session_success() {
    let conn = DB_POOL.get().unwrap();
    let mut user = NewUser::new("[email protected]", "correcthorsebatterystaple", "Foo Bar").save(&*conn);
    user.status = User::STATUS_ACTIVE;
    user.update(&*conn);

    let rocket = api::build_rocket();
    let req_body = json!({
        "email": "[email protected]",
        "password": "correcthorsebatterystaple"
    });
    let mut req = MockRequest::new(Method::Post, "/api/sessions")
        .header(ContentType::JSON)
        .body(&req_body.to_string());
    let mut response = req.dispatch_with(&rocket);
    assert_eq!(response.status(), Status::Ok);
    let resp_body: HashMap<String, Value> =
        serde_json::from_str(&response.body().and_then(|b| b.into_string()).unwrap()).unwrap();
    assert!(resp_body.contains_key(&"token".to_string()));
}

What would be the benefit to using strings other than "foo" and "bar", if I just want to ensure that all of the fields are filled in and valid? I can see the benefit for variable names--the code becomes harder to understand if I rename user to foo--but I don't see the benefit for strings.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2017

We should disable this and many other lints if cfg test is on

@clarfonthey clarfonthey changed the title blacklisted-names lint but for string literals Lint for blacklisted-strings, similar to blacklisted-names Apr 2, 2017
@clarfonthey
Copy link
Contributor Author

I've updated the OP to describe this a bit better.

@mcarton mcarton added good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints T-AST Type: Requires working with the AST labels Apr 3, 2017
@clarfonthey clarfonthey changed the title Lint for blacklisted-strings, similar to blacklisted-names Lint for denylisted-strings, similar to denylisted-names Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST
Projects
None yet
Development

No branches or pull requests

6 participants