riker-rs / riker

Easily build efficient, highly concurrent and resilient applications. An Actor Framework for Rust.
https://riker.rs
MIT License
1.02k stars 69 forks source link

Fix clippy module inception #101

Closed igalic closed 4 years ago

igalic commented 4 years ago

by moving foo/foo.rs into foo/mod.rs This addresses https://github.com/riker-rs/riker/issues/97

hardliner66 commented 4 years ago

@igalic There is also a second way of solving this problem. Instead of moving everything into the corresponding mod.rs you could move the contents from the mod.rs into the corresponding named file (e.g.: system.rs) and move that file outside of the directory. Something like this.

I'm not quite sure which of those I would prefer because both seem to have their pros and cons.

Whats your opinion on the mod.rs free version?

leenozara commented 4 years ago

Hi @igalic thanks for digging into this. What we have today stems from my own personal preference to have each major type have its own .rs file, regardless of the directory name grouping relative types. This however hasn’t been the standard practice and now we see Clippy complaining about it. Tokio and other projects have essentially have structured the way you have proposed.

Unless there’s any objections from other people, I’d like to merge this. Did you want to fix the conflicts and we can then get that done?

@hardliner66 thanks for providing that alternative to consider. Really helps that we’re not just going with whatever has been submitted, especially since we don’t yet have an RFC in place. I think we’re good to go with using mod.rs files, it’s one of the standard ways to structure and I feel that having to jump between directories to navigate specific parts, e.g. system, actor, etc, could be frustrating for some dependent on their development environment.

hardliner66 commented 4 years ago

@leenozara I thought about this a bit more and I think my preference goes slightly towords dropping the mod.rs files.

Both ways are standard ways. The version with mod.rs is the default with rust 2015, the version with {name}.rs is the new default with 2018, but the old version is still supported: https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html#no-more-modrs

I feel that having to jump between directories to navigate specific parts, e.g. system, actor, etc, could be frustrating for some dependent on their development environment.

But thats what you don't have to do if you don't use mod.rs. You only need to navigate to a subfolder if it is a sub-module (not the git kind) like actor/props or system/persist. Another big plus of not using mod.rs is, that it's easier to distinguish between files in your editor if you have multiple files open. With mod.rs I might have multiple mod.rs files open and depending on my editor it might be easier or harder to see which one I'm currently in because I also need the path to know which file I'm editing.

Also it gets a bit easier to search (as in filesystem search) for modules because I only have to search for actor.rs instead of actor/mod.rs. Same goes for the "go to file" features in most IDEs.

Anyway, because I like it only slightly more and my editor (vscode) already shows the folder of the file if there is a second file open I'm happy with both versions.

igalic commented 4 years ago

if i find the time today, I'll create a second version of this pr, so we can look at it side by side

but after what @hardliner66 has said, i too am sliding towards no mod.rs

leenozara commented 4 years ago

@hardliner66 Now I get it. As a vscode user also the benefit wasn’t as obvious to me too. I wasn’t aware of this being the 2018 standard either. Thanks for clarifying.

@igalic I agree with your comment. Adopting the 2018 no mod.rs structure is the way to go.

igalic commented 4 years ago

rebased and rust 2018 mod version of this patch is in https://github.com/riker-rs/riker/pull/108

igalic commented 4 years ago

closing since #108 was merged