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

Closes Issue4903 #5058

Merged
merged 3 commits into from
Jan 29, 2020
Merged

Closes Issue4903 #5058

merged 3 commits into from
Jan 29, 2020

Conversation

xiongmao86
Copy link
Contributor

@xiongmao86 xiongmao86 commented Jan 17, 2020

fixes #4903.

Check list:

  • Followed lint naming conventions
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed ./util/dev update_lints
  • Added lint documentation
  • Run ./util/dev fmt

---

changelog: implement lint that warn about single component path imports(use ident;).

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jan 17, 2020
@xiongmao86 xiongmao86 changed the title [CI Skip][WIP] Closes Issue4903 [WIP] Closes Issue4903 Jan 19, 2020
@bors
Copy link
Collaborator

bors commented Jan 23, 2020

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

@xiongmao86 xiongmao86 force-pushed the issue4903 branch 2 times, most recently from d9f9278 to 7147762 Compare January 25, 2020 13:17
@xiongmao86 xiongmao86 force-pushed the issue4903 branch 2 times, most recently from 3c3f469 to b741eb3 Compare January 26, 2020 15:55
@xiongmao86 xiongmao86 changed the title [WIP] Closes Issue4903 Closes Issue4903 Jan 26, 2020
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Should we allow pub use ident; statements? Can this be useful? Maybe if you want to reexport a dependency?

tests/ui/single_component_path_imports.rs Show resolved Hide resolved
@xiongmao86
Copy link
Contributor Author

I never think of reexport case. Reexport should be allowed. I will update the lint logic.

@flip1995
Copy link
Member

flip1995 commented Jan 29, 2020

Allowing pub imports is as easy as

if item.vis.node.is_pub() || item.vis.node.is_pub_restricted() {
return;
}

@xiongmao86
Copy link
Contributor Author

I figure out item.vis.node. is_pub(), what case does item.vis.node.is_pub_restricted() apply to?

@xiongmao86
Copy link
Contributor Author

xiongmao86 commented Jan 29, 2020

The latest nightly doesn't have is_pub_restricted()? I update a couple minutes ago. Wait to see what ci results.

@flip1995
Copy link
Member

The latest nightly doesn't have is_pub_restricted()

Oh, just realized, that only rustc_hir::VisibilityKind has this function. is_pub_restricted are things like pub(crate), pub(super), crate, pub(my_module), ...

@flip1995
Copy link
Member

But I guess pub(crate) krate is also useless, since you can access krate directly anyway. So allowing is_pub should be enough.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM. Can you squash some commitsß After that, this should be ready to merge.

@xiongmao86
Copy link
Contributor Author

xiongmao86 commented Jan 29, 2020

@flip1995, I notice you have being emphasizing squash commits, I am not very sure what makes commits looks good. Are there anything I can read?

@flip1995
Copy link
Member

Oh I just meant that you squash the commits on this branch together with git rebase -i upstream/master¹. For example the two commits 6db5b20 and 0861f0b can be squashed together, since the first commit adds extern crate .. to the test file and the second one removes it again.

What I normally do is 1 commit for the lint implementation/fix, 1 commit for everything tests, and maybe some additional commits for unrelated things (like ./util/dev {fmt,update_lints}, refactors, ...)


¹: This opens your editor where you can pick commits you want to keep and squash commits you want to include in the commit on the line above.

pick fe07ad7a Small refactor of mutable_debug_assertions
squash 8d93e826 Don't trigger `debug_assert_with_mut_call` on `.await`
pick ac3ca610 Add test for `await` in `debug_assert!(..)`

for example would include the second commit in the first commit and keep the 1st and 3rd commit.

@flip1995
Copy link
Member

flip1995 commented Jan 29, 2020

I guess the easiest thing you could do would be

git reset upstream/master
git add clippy_lints/src/single_component_path_imports.rs
git commit -m "..."
git add tests
git commit -m "..."
git add clippy_lints/src/methods clippy_lints/src/types.rs
git commit -m "Run .util/dev fmt"
git add -u
git commit -m "Update lints"

Before you push, do a sanity check with git diff origin/issue4903, which shouldn't have any output

@xiongmao86
Copy link
Contributor Author

Squashed. Thank you for helping me with this pr. @flip1995.

@xiongmao86
Copy link
Contributor Author

Oh the history didn't disappear?

@flip1995
Copy link
Member

Squashed. Thank you for helping me with this pr. @flip1995.

Helping recurring contributors is always a pleasure :)

Oh the history didn't disappear?

GitHub quirk. You have to reload the page 😉

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 29, 2020

📌 Commit ac9f019 has been approved by flip1995

bors added a commit that referenced this pull request Jan 29, 2020
Closes Issue4903

fixes #4903.

Check list:
- [x] Followed [lint naming conventions][lint_naming]
- [x] Added passing UI tests (including committed `.stderr` file)
- [x] `cargo test` passes locally
- [x] Executed `./util/dev update_lints`
- [x] Added lint documentation
- [x] Run `./util/dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints
---

changelog: implement lint that warn about single component path imports(`use ident;`).
@bors
Copy link
Collaborator

bors commented Jan 29, 2020

⌛ Testing commit ac9f019 with merge f69835b...

@bors
Copy link
Collaborator

bors commented Jan 29, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing f69835b to master...

@bors bors merged commit ac9f019 into rust-lang:master Jan 29, 2020
@xiongmao86 xiongmao86 deleted the issue4903 branch January 29, 2020 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint for use ident;
5 participants