scylladb / scylla-rust-driver

Async CQL driver for Rust, optimized for ScyllaDB!
Apache License 2.0
583 stars 103 forks source link

Enforce import style in the project #550

Open wyfo opened 2 years ago

wyfo commented 2 years ago

This is a pure code style issue.

I've resolved today a merge conflict in transport/cluster.rs because imports had been modified in the meantime by https://github.com/scylladb/scylla-rust-driver/commit/9bf4f74f421faf76122b30c78ec61299fba1783a.

The issue here was

use crate::transport::connection::{Connection, VerifiedKeyspaceName};
use crate::transport::connection_pool::PoolConfig;
use crate::transport::errors::QueryError;
use crate::transport::node::Node;
use crate::transport::topology::{Keyspace, Metadata, MetadataReader};

against

use crate::transport::{
    connection::{Connection, VerifiedKeyspaceName},
    connection_pool::PoolConfig,
    errors::QueryError,
    node::Node,
    session::AddressTranslator,
    topology::{Keyspace, Metadata, MetadataReader},
};

I don't want to argue about the style itself, but to avoid this kind of conflict, I suggest to enforce the import style, for example in CONTRIBUTING.md.

psarna commented 2 years ago

@wyfo feel free to post a PR which adds a corresponding paragraph in CONTRIBUTING.md. As long as default cargo fmt is fine with your proposal, I am too (: @piodul?

piodul commented 2 years ago

Yes, enforcing a consistent import style is a good idea. I did some research and it turns that we can go further and enforce them in CI - you can provide additional options in the .rustfmt.toml file (reference). I believe we would be interested in the following options:

imports_granularity = "Module"
group_imports = "StdExternalCrate"

However, these options are:

I think we should try adding those checks to CI, but only after we close most of the current actively developed/reviewed PRs. Not sure if the paragraph in CONTRIBUTING.md would be needed after that, but I guess it won't hurt.

Thoughts? @psarna @wyfo

psarna commented 2 years ago

Sounds great, nightly isn't an issue for CI indeed, we can add another job that does validation on a nightly toolchain. A paragraph in CONTRIBUTING.md can't hurt either way, but it's not mandatory after the rules are enforced by our CI.

wyfo commented 2 years ago

Yes, it sounds great. Does the choice imports_granularity = "Module" vs imports_granularity = "Crate" need to be debated? :innocent: I know that TiKV (and @wprzytula) uses the second, and I tend to agree with them. Module formatting, on the other hand, seems more common, and would surely reformat less code.