-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 modules from kebab-case to snake_case #14439
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,8 @@ files, place a `main.rs` file along with the extra [*modules*][def-module] | |
within a subdirectory of the `src/bin`, `examples`, `benches`, or `tests` | ||
directory. The name of the executable will be the directory name. | ||
|
||
Binaries, examples, benches and integration tests follow `kebab-case` naming style. Modules within those targets are `snake_case` following the [Rust standard](https://rust-lang.github.io/rfcs/0430-finalizing-naming-conventions.html). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is likely best left to a style guide, with us finding an appropriate place to link out to it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a style guide that makes sense to use? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way I see it, right now no one in the rust ecosystem directly says that build targets are |
||
|
||
You can learn more about Rust's module system in [the book][book-modules]. | ||
|
||
See [Configuring a target] for more details on manually configuring targets. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reply to #14439 (comment):
Yes. See https://doc.rust-lang.org/nightly/cargo/reference/cargo-targets.html.
What's the benefit of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is without a codified standard its hard to enforce. So people will start drifting and you will end up with multiple groups doing different things. If it becomes part of the docs then its easier to PR lints into Clippy in order to enable automatic enforcement for projects and makes the ecosystem easier and more coherent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this lint should live in cargo, not clippy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. However we still need to spell out the rule in some docs first and I think this is the place for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that this is the place for it.
Also, even better than documenting this is to lint for it. If we have specific naming recommendations, we can use #12235 (unstable implementation is in place) to lint for naming like rustc does for functions, structs, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree. However you end up in a chicken and egg problem. If I propose a semantic lint like this with no docs to support it, then I will end up with a really hard time getting it merged. So its just a first step here to remove an ambiguous situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So where specifically should I try to put this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo/src/cargo/core/workspace.rs
Line 1205 in d7bffc3