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

Project's current formatting makes it hard to contribute non-trivial changes #24

Open
regexident opened this issue Dec 15, 2023 · 4 comments

Comments

@regexident
Copy link
Contributor

The project's current code formatting suffers from a couple of issues, affect:

tl;dr: For the sake of making contributions hassle-free it would arguably be preferable to revert to Rust's default format (as applied by rustfmt without a custom rustfmt.toml file) and used by >90% of Rust projects.

  1. It uses a non-standard format

    The Rust project recommends using the default formatting configuration to ease collaboration.

  2. It provides a custom rustfmt.toml file but doesn't actually use it

    While the project provides a rustfmt.toml file

    ./rustfmt.toml

    # https://github.com/rust-lang/rustfmt/blob/master/Configurations.md
    tab_spaces = 3
    fn_call_width = 120
    chain_width = 120
    max_width = 120
    fn_single_line = true

    Running cargo fmt results in 30+ reformatted files, which should not happen if the code had previously been formatted with it.

    As such it is currently impossible to have rustfmt/cargo fmt apply the same formatting as used by the project. And anybody opening a PR on the project gets a nasty surprise when they notice that their IDE auto-formatted the code (as is usually desirable) and now the semantic changes are lost in thousands of formatting changes (as it just happened to me, again).

  3. The rustfmt.toml file requires nightly and is also invalid

    Further more running cargo fmt results in a bunch of warnings:

    $ cargo fmt
    
    Warning: can't set `fn_single_line = true`, unstable features are only available in nightly channel.
    Warning: can't set `fn_single_line = true`, unstable features are only available in nightly channel.
    `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
    `chain_width` cannot have a value that exceeds `max_width`. `chain_width` will be set to the same value as `max_width`
    Warning: can't set `fn_single_line = true`, unstable features are only available in nightly channel.
    `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
    `chain_width` cannot have a value that exceeds `max_width`. `chain_width` will be set to the same value as `max_width`
    `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
    `chain_width` cannot have a value that exceeds `max_width`. `chain_width` will be set to the same value as `max_width`
    `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
    `chain_width` cannot have a value that exceeds `max_width`. `chain_width` will be set to the same value as `max_width`
    `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
    `chain_width` cannot have a value that exceeds `max_width`. `chain_width` will be set to the same value as `max_width`
    
  4. The project mixes CRLF and LF

    • 37 files use CRLF
    • 31 use LF.
@s-arash
Copy link
Owner

s-arash commented Dec 17, 2023

You are correct! I need to merge the byods branch before fixing formatting issues. I don't think I'll switch to the default rustfmt config though.

@regexident
Copy link
Contributor Author

I don't think I'll switch to the default rustfmt config though.

As long as there's a corresponding rustfmt.toml provided by the repo 👍.

@mkatychev
Copy link
Contributor

You can use a nightly rustfmt config values in a rust-stable codebase by sticking rustfmt.toml in a non-root dir:

rustup run nightly cargo fmt -- --config-path fmt/rustfmt.toml

@s-arash
Copy link
Owner

s-arash commented Mar 9, 2024

@mkatychev Thanks for the suggestion. I'll get it once I get a chance to merge the BYODS work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants