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

rename {submodule}.rs with corresponding {submodule} directory to {submodule}/mod.rs #771

Closed
rnbguy opened this issue Jul 17, 2023 · 4 comments · Fixed by #975
Closed

rename {submodule}.rs with corresponding {submodule} directory to {submodule}/mod.rs #771

rnbguy opened this issue Jul 17, 2023 · 4 comments · Fixed by #975
Assignees
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding
Milestone

Comments

@rnbguy
Copy link
Collaborator

rnbguy commented Jul 17, 2023

Improvement Summary

Rename all {submodule}.rs with a corresponding {submodule}/ to {submodule}/mod.rs. This doesn't change anything in the rust namespace.

Example.

crates/ibc/src/applications/transfer/msgs/ and crates/ibc/src/applications/transfer/msgs.rs exist. So, I propose to rename the rust file to crates/ibc/src/applications/transfer/msgs/mod.rs.

Proposal

Usually the file explorers sort the directories first and then the files. So, maintaining {submodule}.rs and {submodule}/ directory makes them separated on the viewing list.

fd -e rs -x ls -d {.} 2> /dev/null provides a list of directories which falls under this.

@rnbguy
Copy link
Collaborator Author

rnbguy commented Jul 17, 2023

Semi-counter argument to my proposal.
https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs

But I still stand by my proposal - {submodule}.rs should be under {submodule}/.

@plafer
Copy link
Contributor

plafer commented Jul 18, 2023

Semi-counter argument to my proposal.
https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs

This is why I don't like mod.rs; whenever I search for a file, I see a bunch of mod.rs (and must visually parse the directory it's in), as opposed to just looking for e.g. msgs.rs.

So I'm of the opposite opinion: we should get rid of all mod.rs

@rnbguy
Copy link
Collaborator Author

rnbguy commented Jul 18, 2023

whenever I search for a file, I see a bunch of mod.rs (and must visually parse the directory it's in), as opposed to just looking for e.g. msgs.rs.

Usually the file searches are done via fuzzy search, but I see your point. It is one of the reasons, VSCode shows the parent names if files with same name are opened.

image

In my opinion, with mod.rs, I don't have to go up in the directory to look for submodule root. With submodule.rs, I have to go up and type the correct name.

@Farhad-Shabani
Copy link
Member

My view: mod.rs provides cleaner and better organization, especially as a project grows.
More self-contained. Easier to browse through files and see all its submodules at a glance. This is particularly useful when its main purpose is serving as a parent of submodules without containing functions. Then, it’s a good place to handle encapsulation and see what a module offers to the whole library. This way, we won’t be looking for types/functions there anymore.
Though, should admit, deciding on this feels like it might come down to individual preferences. 🙂

@Farhad-Shabani Farhad-Shabani added O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews labels Jul 18, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.48.0 milestone Nov 24, 2023
@Farhad-Shabani Farhad-Shabani self-assigned this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants