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

Remove unnecessary path prefixes #10749

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

tygyh
Copy link
Contributor

@tygyh tygyh commented Nov 26, 2023

Objective

  • Shorten paths by removing unnecessary prefixes

Solution

  • Remove the prefixes from many paths which do not need them. Finding the paths was done automatically using built-in refactoring tools in Jetbrains RustRover.

@Nilirad Nilirad added the C-Code-Quality A section of code that is hard to understand or change label Nov 26, 2023
@Nilirad
Copy link
Contributor

Nilirad commented Nov 26, 2023

Looks interesting but in some cases it seems quite aggressive. For example, Result prefix is sometimes helpful to know as there are many Result types. Also I have mixed feelings about stripping out the enum type when specifying a variant.

@tygyh
Copy link
Contributor Author

tygyh commented Nov 26, 2023

I have changed 119 files, if nothing was controversial I would be concerned. :D

I already knew I would have to revert some parts. Just tell me what. And if it's too much work to go through it all I can break up the Pull Request into several smaller.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good changes. I'd like to get this through ASAP to avoid killing you with merge conflicts.

Some additional cleanup you can do, and I would really like a clippy lint for this.

I actually quite like not using path imports for std::Result. IMO that should be saved to call out unusual Result types that are punning. (Don't do this! Bad API!)

@tygyh
Copy link
Contributor Author

tygyh commented Nov 27, 2023

Some additional cleanup you can do, and I would really like a clippy lint for this.

Do you want me to add a clippy lint? I have no idea how to do that.

@alice-i-cecile
Copy link
Member

Lints are added at the workspace level here.

Now, let's check the list of clippy lints to see if anything matches...

The absolute_paths lint is closest but not quite the same. Let me open an issue!

@smoelius
Copy link

@alice-i-cecile I just wanted to point you to Dylint's inconsistent_qualification lint. It's very similar to the lint you proposed in rust-lang/rust-clippy#11888.

Please forgive me jumping on your PR.

@alice-i-cecile
Copy link
Member

Very useful, no worries! I'm a bit nervous about adding another tool to our CI for now, but I'll keep it on my radar :)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 28, 2023
@alice-i-cecile
Copy link
Member

@tygyh once you can get this passing CI and free of merge conflicts, I'll merge this as trivial <3

@tygyh
Copy link
Contributor Author

tygyh commented Nov 28, 2023

@alice-i-cecile It is ready now!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 28, 2023
Merged via the queue into bevyengine:main with commit fd30857 Nov 28, 2023
22 checks passed
@tygyh tygyh deleted the remove-path-prefixes branch November 29, 2023 10:23
james7132 pushed a commit to james7132/bevy that referenced this pull request Dec 1, 2023
# Objective

- Shorten paths by removing unnecessary prefixes

## Solution

- Remove the prefixes from many paths which do not need them. Finding
the paths was done automatically using built-in refactoring tools in
Jetbrains RustRover.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants