Skip to content

Add unnecessary_import_braces lint. #16931

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

Merged
merged 1 commit into from
Sep 17, 2014
Merged

Add unnecessary_import_braces lint. #16931

merged 1 commit into from
Sep 17, 2014

Conversation

omasanori
Copy link
Contributor

This PR adds a lint path checking unnecessary brackets around single imported item. (ex. use std::num::{abs};)

I was unsure whether the lint should be allow-by-default or warn-by-default, so I split changes into two commits. If the lint itself is OK but it shouldn't be warn-by-default, then I'll purge the second one. What do you think?

@sinistersnare
Copy link
Contributor

could it check for unneccessary brackets around closures?

fn do_a_thing() -> || -> int {
    || { 2 } // This warns
}

@omasanori
Copy link
Contributor Author

@sinistersnare No, I've never be aware of such unnecessary brackets.

@sinistersnare
Copy link
Contributor

that actually does not compile, but something like that. hopefully someone can give a working example, i just wanted to throw in my 2 cents.

@omasanori
Copy link
Contributor Author

I'll try to check such expressions if it is also desired. Any working example should be nice.

I wrote this lint because I always search commas when I see use foo::{very_long_single_identifier}; and I found that unnecessary brackets in use statement might be noise.

@sinistersnare
Copy link
Contributor

I agree that this might be useful, but it can have more uses that originally intended too!

@omasanori
Copy link
Contributor Author

Indeed. I'll dig into libsyntax to find more and more unnecessary brackets...

@ftxqxd
Copy link
Contributor

ftxqxd commented Sep 2, 2014

The name of this lint is slightly confusing—unnecessary_parens already exists (for things like if (condition) { ... }), and I (and most people I’ve heard talk about them) normally use the term ‘braces’ to describe { and }. Maybe it could be renamed unnecessary_braces or even unnecessary_import_braces?

I also think that this shouldn’t detect unnecessary braces around closures. I’ve seen a lot of code that uses braces when it doesn’t strictly need to, but it really does make it clearer and easier to add more statements, especially if it spans multiple lines. If such a lint were to be added, it should be part of the unnecessary_parens lint (which might have to be renamed).

But overall, this is a good idea. I see unnecessary {}s in imports all the time, and it’s quite annoying.

@jfager
Copy link
Contributor

jfager commented Sep 2, 2014

'Brackets' are []. As @P1start said, this is for 'braces' or 'curly brackets': http://en.m.wikipedia.org/wiki/Bracket

I vote for allow-by-default. With globs being discouraged I've reflexively started using braces anticipating that I'll need to add imports as I go.

@omasanori
Copy link
Contributor Author

I'm sorry for inaccurate wording. The lint is renamed to unnecessary_import_braces.

@omasanori
Copy link
Contributor Author

@sinistersnare After a minute of thought, I have an opinion that redundant braces around expressions and ones in use statements are different kinds of style issue.
I'm unsure whether putting extra braces around expressions is good style or bad style, but at least I think it should be treated in any other lint. Your opinion?

@omasanori omasanori changed the title Add unnecessary_brackets lint. Add unnecessary_import_braces lint. Sep 2, 2014
The lint checks any unnecessary braces around one imported item like
`use std::num::{abs};`.

Signed-off-by: OGINO Masanori <[email protected]>
@omasanori
Copy link
Contributor Author

I decided to purge the second commit and make the lint allow-by-default for now. r?

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 17, 2014
@bors bors merged commit 6113520 into rust-lang:master Sep 17, 2014
@omasanori omasanori deleted the unnecessary-path-brackets branch September 17, 2014 22:31
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

Successfully merging this pull request may close these issues.

5 participants