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

Stuttering #66

Open
dtolnay opened this Issue May 18, 2017 · 19 comments

Comments

Projects
None yet
10 participants
@dtolnay
Copy link
Member

dtolnay commented May 18, 2017

Should it be log::Level or log::LogLevel? This comes up all the time and we've never really settled it as a community. So far we have tended to encourage importing types rather than modules:

use log::LogLevel; // encouraged

let level: log::LogLevel; // discouraged

But this works less well if the type relies on the module name to disambiguate what it means, as in the case of io::Result.

use std::io::Result; // probably not what you want in most cases

To add to the mess, we have tended to discourage renaming imports.

use std::io::Result as IoResult; // discouraged

How do we reconcile all of these?

@dtolnay

This comment has been minimized.

Copy link
Member Author

dtolnay commented May 18, 2017

Clippy currently has a lint against types that stutter, but it is disabled by default.

@dtolnay

This comment has been minimized.

Copy link
Member Author

dtolnay commented May 18, 2017

There might be some guidance from the C++ world. In particular, Google's C++ style guide disallows using namespace so they must have conventions to avoid stutter.

@dpc

This comment has been minimized.

Copy link

dpc commented May 18, 2017

IMO, Go forcing everything imported to be qualified by import name did the right thing.

@seanmonstar

This comment has been minimized.

Copy link

seanmonstar commented May 19, 2017

An interesting example is the std::fmt module. You typically are not encouraged to import types from the module, but just import the module itself and qualify all usage. For example:

impl fmt::Display for Foo {
    fn fmt(&self, f: &muy fmt::Formatter) -> fmt::Result {
        // ...
    }
}
@dtolnay

This comment has been minimized.

Copy link
Member Author

dtolnay commented May 19, 2017

That's an interesting one. For whatever reason, in my code I have found that I prefer this:

use std::fmt::{self, Debug, Display};

impl Display for Foo {
    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        /* ... */
    }
}
@daboross

This comment has been minimized.

Copy link
Contributor

daboross commented May 20, 2017

I think this should provide at least one canonical guidance for this: https://github.com/rust-lang/rfcs/blob/master/text/0356-no-module-prefixes.md. It's about modules rather than crates, but I think the same reasons apply?

I originally saw this convention at https://aturon.github.io/style/naming/README.html#avoid-redundant-prefixes-[rfc-356] too.

Note that the RFC mentions use io::Error as IoError; too - I believe as a "good" workaround?

@seanmonstar

This comment has been minimized.

Copy link

seanmonstar commented May 20, 2017

As for renaming imports, the most common place I do that is use std::error::Error as StdError;.

@Kixunil

This comment has been minimized.

Copy link

Kixunil commented May 31, 2017

I prefer use std::fmt. Same with io. I dislike stuttering.

@budziq

This comment has been minimized.

Copy link

budziq commented Jun 4, 2017

Hmm, in reqwest I can see
redirect::{RedirectPolicy, RedirectAction, RedirectAttempt};
This could also qualify as stuttering.

@seanmonstar

This comment has been minimized.

Copy link

seanmonstar commented Jun 4, 2017

True, though the naming was because I don't expect to expose the internal "redirect" module, so they can just be 'Policy' and so on. I suppose they could internally, with a public reexport introducing the prefix, but either option doesn't seem much better in the end.

@budziq

This comment has been minimized.

Copy link

budziq commented Jun 4, 2017

So going further with reqwest as an example. Assuming that Redirect module is made public seanmonstar/reqwest#105 should its types be "destuttered" by default along with the export?

Or is certain level of repetition is still encouraged as is the case Client::ClientBuilder / Request::RequestBuilder?

@jonas-schievink

This comment has been minimized.

Copy link

jonas-schievink commented Jul 22, 2017

This came up in kyren/rlua#15, too.

To add to the mess, we have tended to discourage renaming imports.

That'd make rluas API a lot more noisy to use, since it would export a lot of generic types (including String) if the Lua prefix is removed.

@Kixunil

This comment has been minimized.

Copy link

Kixunil commented Jul 26, 2017

@jonas-schievink LuaString, LuaResult, etc seem like a reasonable exceptions to the rule to me.

@carllerche

This comment has been minimized.

Copy link

carllerche commented Aug 8, 2018

Any conclusion? I need answers, but have no thoughts :)

@tshepang

This comment has been minimized.

Copy link
Contributor

tshepang commented Aug 8, 2018

Not stuttering is good... I like:

use std::io;
fn foo() -> io::Result<()> {
...
@tshepang

This comment has been minimized.

Copy link
Contributor

tshepang commented Aug 8, 2018

Renames are fine too, so long as they don't stutter. Also, I would not love seeing this (which actually stutters too):

use std::error::Error as StdError;
@dtolnay

This comment has been minimized.

Copy link
Member Author

dtolnay commented Aug 8, 2018

People will use module-qualified paths if they are sensible and pervasive in your documentation and usage examples. "Sensible" is a function of number of characters typed and how unique the name is.

In my experience people are fine typing 2-3 character modules and mostly fine with 4 character modules. More than that is asking too much, meaning that items in a 5+ character module or library better have a name that can stand alone.

  • carl::Options -- if your documentation writes it this way pervasively, users typically will as well.
  • zx::Options -- practically all users will be okay with this.
  • carl::options::Options -- this is probably the worst. Users cannot import carl::options and then deal with options::Options because options::Options is dumb. Users cannot import carl::options::Options because it looks too much like prelude's Option. Users are forced to import and rename to CarlOptions or something.
  • carl_framework::Options -- few people will type this any more often than twice in a file, even if your documentation uses it. Framework will need to give Options a more unique name so users can write an import.
  • carl_framework::CarlOptions -- this is fine, users will import. Better than carl_framework::Options.
  • carl::CarlOptions -- worse than carl::Options.

Certain items have somewhat higher thresholds between sensible and not sensible. For Error and Result types I would say the threshold is 8-10 characters in the library name (the length up to which users happily write a qualified path, even if used several times in a file). For free functions the threshold is 12-15. Again, documentation needs to be consistent in order for them to do it that way.

@softprops

This comment has been minimized.

Copy link

softprops commented Sep 11, 2018

I've recently been thinking about this specifically in the context of errors https://twitter.com/softprops/status/1038234080305967105?s=19

Being my own devils advocate what this can lead to is overscoping of errors on focused methods. In other words a method in a foo crate that returns foo::Error but in practice may only yield a subset of the total types of errors foo::Error represents. This means clients are asked to handle a representation of errors that won't ever occur in a specific context. Yes this encourages much more focused crates where foo::Error enumerates a very small set of cases but in practice this seems rarely the case. foo::Error typically represents all the cases.

@dtolnay

This comment has been minimized.

Copy link
Member Author

dtolnay commented Sep 11, 2018

The discussion of fine-grained vs broader error types is in #46.

If a library or module uses fine-grained errors like the ones returned by std FromStr impls that you linked to, I would expect that they would not all be called Error. They would also not be called foo::FooError because by definition of being fine-grained they are characterized by more than simply "the error associated with library foo" -- so you probably wouldn't run into a problem of stutter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.