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

Move semicolon_if_nothing_returned lint to pedantic #7148

Closed
wants to merge 4 commits into from
Closed

Move semicolon_if_nothing_returned lint to pedantic #7148

wants to merge 4 commits into from

Conversation

mbartlett21
Copy link
Contributor

@mbartlett21 mbartlett21 commented May 1, 2021

This moves the semicolon_if_nothing_returned lint to pedantic category.

changelog: Move [semicolon_if_nothing_returned] lint into pedantic category.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @phansch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 1, 2021
@mbartlett21 mbartlett21 changed the title Update semicolon_if_nothing_returned.rs Move semicolon_if_nothing_returned lint to style May 1, 2021
@flip1995
Copy link
Member

flip1995 commented May 1, 2021

What's the motivation in moving this to style?

@mbartlett21
Copy link
Contributor Author

According to the formatting guide, this lint is enforcing the preferred style, so I think it should be in that category, rather than restriction.

Use a semi-colon where an expression has void type, even if it could be propagated. E.g.,

fn foo() { ... }

fn bar() {
    foo();
}

https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/statements.md

@flip1995
Copy link
Member

flip1995 commented May 1, 2021

Since the rust style guidelines recommend that, I guess we could move it to style. But at the same time I feel like this is really pedantic for a warn-by-default lint. So if we move this, we should only move it to pedantic. I'm open for different opinions though.

@camsteffen
Copy link
Contributor

camsteffen commented May 2, 2021

That style guide is in fmt-rfcs which seems to be in a sort of limbo-state, not really an accepted official style guide. Also those guidelines are mainly intended for rustfmt - rules that can be automatically enforced by a formatter. This is a bit of a unique rule since it is "type-aware" which is why Clippy adopted it. So the weird RFC status combined with the "pickiness" makes it fit into pedantic IMO.

Would this be a good time to consider a rename or is it too late? The current name reads badly as allow(semicolon_if_nothing_returned). I'd suggest unit_statements_without_semicolon.

@giraffate
Copy link
Contributor

ping from triage @mbartlett21. Is there anything you still want to discuss?

@mbartlett21
Copy link
Contributor Author

@giraffate
Thinking over this again, I've now moved this instead to pedantic.

@mbartlett21 mbartlett21 changed the title Move semicolon_if_nothing_returned lint to style Move semicolon_if_nothing_returned lint to pedantic May 19, 2021
@bors
Copy link
Contributor

bors commented May 20, 2021

☔ The latest upstream changes (presumably #7253) made this pull request unmergeable. Please resolve the merge conflicts.

@mbartlett21
Copy link
Contributor Author

mbartlett21 commented May 24, 2021

Now I realise that I should probably not be doing this on my master branch, since I can't rebase very well. I will redo in another branch and make another pr and link it.

EDIT: See #7268

bors added a commit that referenced this pull request May 24, 2021
Move `semicolon_if_nothing_returned` to `pedantic`

This moves the `semicolon_if_nothing_returned` lint to `pedantic` category.
I had done #7148, but on the master branch, and Github doesn't seem to let me change that, so here's another PR

changelog: Move [`semicolon_if_nothing_returned`] lint into `pedantic` category.
bors added a commit that referenced this pull request May 25, 2021
Move `semicolon_if_nothing_returned` to `pedantic`

This moves the `semicolon_if_nothing_returned` lint to `pedantic` category.
I had done #7148, but on the master branch, and Github doesn't seem to let me change that, so here's another PR

changelog: Move [`semicolon_if_nothing_returned`] lint into `pedantic` category.
bors added a commit that referenced this pull request May 25, 2021
Move `semicolon_if_nothing_returned` to `pedantic`

This moves the `semicolon_if_nothing_returned` lint to `pedantic` category.
I had done #7148, but on the master branch, and Github doesn't seem to let me change that, so here's another PR

changelog: Move [`semicolon_if_nothing_returned`] lint into `pedantic` category.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants