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

Add TOML formatting with Taplo #784

Merged
merged 7 commits into from
Dec 12, 2024
Merged

Add TOML formatting with Taplo #784

merged 7 commits into from
Dec 12, 2024

Conversation

PoignardAzur
Copy link
Contributor

Add Taplo config.
Format TOM config files.
Add Taplo step to CI.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's bump the column width to 100 and the indentation size to 4 spaces. Both of these values are exactly what rustfmt does with Rust code.

.taplo.toml Show resolved Hide resolved
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get a second opinion from @xStrom on the actions design, but otherwise this looks right to me.

@@ -81,6 +81,14 @@ jobs:
- name: Run cargo fmt
run: cargo fmt --all --check

- name: Install Taplo
uses: uncenter/setup-taplo@v1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of using another third party actions provider, but I see that wgpu also uses this.

It doesn't seem like the maintainer of this action actually uses it, which is slightly concerning.
If we just depended on a specific hash rather than just @v1, the security concern goes away

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this wouldn't have been my immediate reaction if we used the taiki-e/install-action here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked into adding taplo to the install-action list, but the format of taplo's release artifacts seems to change from release to release, so I'm not comfortable taking on that work.

Using a specific hash for uncenter's action seems easy enough.

with:
version: "0.9.3"

- name: run `taplo fmt`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: run `taplo fmt`
- name: Run taplo fmt

would be consistent with the other projects.

.taplo.toml Outdated

[formatting]
# This is a matter of taste, but expanded inline arrays make tables harder to read.
array_auto_collapse = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to already be the default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah not much point in defining array_auto_collapse here as it matches the default.


# Aligning comments with the largest line creates
# diff noise when neighboring lines are changed.
align_comments = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

.taplo.toml Outdated
Comment on lines 5 to 7
# This is a matter of taste, but expanded inline arrays make tables harder to read.
array_auto_collapse = true
inline_table_expand = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about setting inline_table_expand to false. The expansion only happens when the line exceeds the length defined in column_width (which we should have at 100).

With it false, the following happens:

winapi = { version = "0.3.9", features = [
    "winbase",
    "libloaderapi",
    "errhandlingapi",
    "winuser",
    "shellscalingapi",
    "shobjidl",
    "combaseapi",
    "dxgi1_3",
    "dwmapi",
    "wincon",
    "fileapi",
    "processenv",
    "winbase",
    "winerror",
    "handleapi",
    "shellapi",
    "winnls",
] }

# .. gets converted into:

winapi = { version = "0.3.9", features = ["winbase", "libloaderapi", "errhandlingapi", "winuser", "shellscalingapi", "shobjidl", "combaseapi", "dxgi1_3", "dwmapi", "wincon", "fileapi", "processenv", "winbase", "winerror", "handleapi", "shellapi", "winnls"] }

Also, just so I understand your claim, are you saying that the following expansion that we currently have makes the [workspace.lints] table hard to read?

rust.unexpected_cfgs = { level = "warn", check-cfg = [
    'cfg(FALSE)',
    'cfg(tarpaulin_include)',
] }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found while testing it that the formatter was really eager to break up inline tables even when it didn't seem needed to stay below the column limit. I'll give it another look.

@PoignardAzur
Copy link
Contributor Author

I've addressed review comments.

Taplo with a column width of 100 seems to mostly leave inline tables be, so I've removed the inline_table_expand = false part.

@xStrom
Copy link
Member

xStrom commented Dec 12, 2024

The config options look good now. Still two things though:

  • The CI step name should not have backticks, they won't be converted into CSS.
  • One TOML file needs adjustment to pass the CI. 😆

@PoignardAzur
Copy link
Contributor Author

Huh, I really thought I'd fixed both.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@PoignardAzur PoignardAzur added this pull request to the merge queue Dec 12, 2024
Merged via the queue into main with commit fb27ac7 Dec 12, 2024
17 checks passed
@PoignardAzur PoignardAzur deleted the taplo branch December 12, 2024 16:40
@alerque
Copy link

alerque commented Dec 12, 2024

the indentation size to 4 spaces

Why go with something different than what cargo itself generates? That's just going to be an ongoing conflict with other tools. TOML is not Rust, I would suggest 2 space indent to match other tools that edit the manifest and lock files in the Rust toolchain.

@xStrom
Copy link
Member

xStrom commented Dec 12, 2024

Well we had far more 4 spaces already for one.

I am curious though which cargo command generates indents? I know Cargo.lock has them, but there it's only a single space.

I tried cargo add tokio --features bytes,fs,full,io-std,io-util,libc,macros,mio,net,parking_lot,process,rt,signal,socket2,sync,test-util,time,tokio-macros,tracing and it put everything on a single line, which is definitely not great.

tokio = { version = "1.42.0", features = ["bytes", "fs", "full", "io-std", "io-util", "libc", "macros", "mio", "net", "parking_lot", "process", "rt", "signal", "socket2", "sync", "test-util", "time", "tokio-macros", "tracing"] }

@xStrom
Copy link
Member

xStrom commented Dec 12, 2024

The Rust Style Guide has a page called Cargo.toml conventions. It states:

Use the same line width and indentation as Rust code.

So there you go, the Rust docs seem to agree with our chosen options of 100 and 4 spaces.

@alerque
Copy link

alerque commented Dec 12, 2024

So there you go

Indeed. I guess I'll take my beef upstream to cargo tooling that doesn't do that. Something I just bumped into a couple weeks ago was reformatting my manifest and I wasn't too happy about it. Maybe it was cargo edit or cargo set-version. I'll check it out.

In any event if that's what the Rust style guide suggests, then unless/until that gets changed upstream I'd say carry on! I'll likely adjust my own taplo usage to match if that's really the case.

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

Successfully merging this pull request may close these issues.

4 participants