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 · 8 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 to it once I get a chance to merge the BYODS work.

@regexident
Copy link
Contributor Author

@s-arash it looks like the BYODS work got merged in the meanwhile. Any update on this?

@mkatychev
Copy link
Contributor

mkatychev commented Oct 30, 2024

This is a bit ancillary but you can also define your own formatting rules using topiary as well, there's rust support under the hood but this specifically allows one to apply custom formatting for ascent grammar through s-expressions:
https://topiary.tweag.io/playground/

I've been working on a formatter for OpenSCAD using topiary with great success and have been meaning to finish https://github.com/mkatychev/tree-sitter-ascent sometime this month:
Leathong/openscad-LSP#33 (comment)

@s-arash
Copy link
Owner

s-arash commented Nov 6, 2024

Thanks for the reminder @regexident! I'll get to it soon.

@s-arash
Copy link
Owner

s-arash commented Nov 6, 2024

Very cool @mkatychev, good luck!

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