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 modules from kebab-case to snake_case #14439

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DSchroer
Copy link

@DSchroer DSchroer commented Aug 21, 2024

What does this PR try to resolve?

It is currently unclear reading the docs how files should be named. RFC 430 is clear on crates and module names, kebab-case is never mentioned and while there are historical considerations, we should guide new development towards snake_case. We should align the documented defaults to that RFC.

How should we test and review this PR?

Review the docs and determine if we view value in the change.

@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2024
Comment on lines 14 to 16
│ ├── named_executable.rs
│      ├── another_executable.rs
│      └── multi_file_executable/
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this would have end-user affects, making the default binary names use _ when the status quo is for binaries to use -.

As these are not just mod names but build target names, i would be inclined to leave all of these as -

@ehuss
Copy link
Contributor

ehuss commented Aug 21, 2024

Thanks for the PR! I think there may be some confusion, as these filenames do not represent crate names, they are target names. Considering that there isn't a standard for how those should be named, I'm disinclined to change these here. Indeed, cargo's own repository makes use of hyphen in most places.

@DSchroer
Copy link
Author

Considering that there isn't a standard for how those should be named, I'm disinclined to change these here. Indeed, cargo's own repository makes use of hyphen in most places.

Should we call that out in this page then? I am happy with kebab-case for build targets as they do seem out of scope from that RFC. However we should spell it out to avoid future confusion.

@DSchroer
Copy link
Author

I have tried to add some clarity by explicitly stating my current understanding here. What do you think?

One thing I debated was if using a single word for targets should be recommended but that does not feel right.

@@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

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

Is there a style guide that makes sense to use?

Copy link
Author

Choose a reason for hiding this comment

The 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 kebab-case. This document is the closest but it is not clear.

@DSchroer
Copy link
Author

DSchroer commented Aug 22, 2024

Are cargo targets a cargo concept? If so can we define the naming convention somewhere here in the cargo docs that says those targets should be kebab-case?

@DSchroer DSchroer requested a review from epage August 22, 2024 20:01
Copy link
Member

@weihanglo weihanglo Aug 23, 2024

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):

Are cargo targets a cargo concept?

Yes. See https://doc.rust-lang.org/nightly/cargo/reference/cargo-targets.html.

If so can we define the naming convention somewhere here in the cargo docs that says those targets should be kebab-case?

What's the benefit of it?

Copy link
Author

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.

Copy link
Member

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

I suspect this lint should live in cargo, not clippy.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

Also, even better than documenting this is to lint for it.

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.

Copy link
Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation 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.

6 participants