[Feature] Miscellaneous code tidying

[hdwalters]

The Amber source code contains some features which make it inconvenient to maintain; I would like to do some (minor but widespread) refactoring to fix this:

1. Some use import statements contain unnecessarily nested braces, for example in parentheses.rs:

use heraclitus_compiler::prelude::*;
use crate::{docs::module::DocumentationModule, modules::{prelude::FragmentKind, types::{Type, Typed}}, utils::metadata::ParserMetadata};
use crate::translate::module::TranslateModule;
use super::expr::Expr;

I would like to split these out, so we have at most one pair of braces per use statement, at the "leaf" level of the module tree; and also sort them with grouping on super and crate. RustRover has tools to help with this:

use super::expr::Expr;
use crate::docs::module::DocumentationModule;
use crate::modules::prelude::FragmentKind;
use crate::modules::types::{Type, Typed};
use crate::translate::module::TranslateModule;
use crate::utils::metadata::ParserMetadata;
use heraclitus_compiler::prelude::*;

2. In Rust, multiline comma separated lists (e.g. enum, struct and trait definitions, import statements, function definition parameters, function call arguments, and anything else aside from macro expansions) can have a trailing comma on the final element, so we could add a comma to Generic in the following enum definition in types.rs:

pub enum Type {
    #[default] Null,
    Text,
    Bool,
    Num,
    Array(Box<Type>),
    Generic
}

This makes subsequent pull requests marginally nicer, because it is then unnecessary to add a comma to the new penultimate item, when adding an item at the end. I've been suggesting we do this on new code, and would like to fix up the legacy code too.

3. I would like to make some other minor changes, such as removing the empty file with from the source code; I suspect this was added by accident in the recent translation layer PR.

[Mte90]

Maybe it is the case to configure clippy or a tool to do this things automatically?

[hdwalters]

Maybe it is the case to configure clippy or a tool to do this things automatically?

I'm not sure I trust an automated process to do that. Besides, I'm hoping that once I've done this once, it's a pattern we can follow in subsequent PRs.

[ArjixWasTaken]

Configuring formatting rules for cargo fmt would be very nice.

Having unrelated changes show up in a commit because it reformated the entire file is unfortunate.

PS: you can opt out of formatting in specific scopes, so in the rare chance you dislike certain formatting, it's not a deal breaker

[hdwalters]

Having unrelated changes show up in a commit because it reformated the entire file is unfortunate

That's why I'm proposing a big change to get it all out of the way in one go. I really don't like the idea of automatic formatting.

[hdwalters]

So on the assumption that noone has any objections to this proposal (do you want to weigh in @Ph0enixKM?) I'll raise a PR.

[Ph0enixKM]

@hdwalters I think that the best way to deal with this issue (and any really) is to solve it in a way so that it never comes up again. In this case I think that adding linter rules to rustfmt should be sufficient. Relying on prettier or any code formatter is not a good idea since it:

• reduces diff code readability
• makes us reliant on the code formatting that it thinks is best even though we might think otherwise
• it's another thing we have to maintain

Helpful resources

1. Here is a rule that lints sorting of import statements.

2. Here is a rule that lints import nesting and can keep these brackets to only one level of scope when used with value Module

[Mte90]

I agree, it is better a tool otherwise you have to check in every PR if everything is sorted instead of investing your time on reviewing the code that is more important.

[hdwalters]

Okay, overruled by pretty much everyone; I know when I'm beat! I'll read up on rustfmt, then do an initial big bang change, and set it up for ongoing development.

[hdwalters]

One problem with this. As far as I can tell, rustfmt is an all-or-nothing tool; I was unable to make it just do the import optimization and trailing commas, and nothing else. Even if we create a rustfmt.toml file with specific options, it's going to touch far more of the source code than I outlined in this issue.

I've been persuaded that it's ok to use this tool to format the source code, especially as it's standard across the Rust community. But I'm wary of making changes which would cause unnecessary pain to other contributors with active PRs.

Further thoughts, anyone?

[ArjixWasTaken]

How about you format all the files that are untouched by any current PRs, that way the PRs can rebase and format their files on their own.

we got 9 PRs that are not marked as drafts

--

as for how you'd ignore specific files, well, you can't, but you can simply discard the changes done to those specific files

[Mte90]

It is a common problem usually on starting using these tools.

So we need a PR that fix all the issue in all the Amber files and later force in the CI rustfmt so everyone will update only about the changes. The real next step should be reviewed all the PR in queue to avoid issues with the code that is not linted.

Another option it is to execute the tool in PR only on the files changed, but this means that we can have PRs of 1 line change with all the file formatted because we didn't format our codebase.

In my opinion, we should first reviews/merge the PR opened and implement rustfmt.

[Ph0enixKM]

cargo fmt --check

Running the above command can be used for CI to check if the formatting is correct (if there are some changes that need to be added, the exit code is non-zero).

Proposed solution

Since we can't add just the things we need, how about this solution:

1. Run cargo fmt --check
2. Check what does rustfmt complains about
3. Exclude these from our fmt config in a rustfmt.toml (unless these are some really good bits)
4. Add the rules for import sorting and all
5. Make sure the cargo fmt --check is executed by CI in github workflow

@hdwalters do you think that this is good enough? If not, let me know what am I missing

[Ph0enixKM]

@hdwalters could you also consider this formatting rule? It'd help a lot!

trailing_comma = "Always"

[lens0021]

I posted indirectly related news here, as the description mentioned import formatting, a drama similar to this has happened at Linux.

Linus Torvalds has raised concerns about Rust's automated formatting tool and pull request text formatting practices in a recent Linux Kernel Mailing List discussion.^1

The issue stems from Rust's formatting rules that automatically decide when to use single-line versus multi-line formatting based on length. He ultimately ignored the formatting checker's suggestions and asked Rust-for-Linux maintainer Miguel Ojeda if there was a "sane solution."^1

Miguel responded with a measured explanation. He noted that rustfmt has configuration options for controlling import formatting, but they're currently unstable and only available in nightly builds.^1

Someone on Reddit described this as "Rustfmt is effectively unmaintained", anyway now a (potentially temporary) rustfmt-contributors team has been made.