Skip to content

Conversation

@ogoffart
Copy link
Member

@ogoffart ogoffart commented Apr 3, 2025

Commit cd6f2e2 reformated the .toml files but the formatter split some line that should have been kept in one line as other scripts were parsing the toml

Give a sane column with to avoid over-splitting of lines.

The broken script was publish_nom_package.yaml that changes the default in api/node/Cargo.toml

@ogoffart ogoffart requested review from hunger and tronical April 3, 2025 07:49
Copy link
Member Author

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

The new column width is more inline with the previous practice.

.mise/tasks.toml Outdated
["fix:toml:format"]
description = "Run taplo format"
run = "taplo format"
run = "taplo format --option column_width=120"
Copy link
Member Author

Choose a reason for hiding this comment

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

This line was manually edited

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please put that into .taplo.toml so that tooling will pick that up consistently? The VSCode extension of taplo will just ignore the change here as will the taplo LSP.

"backend-qt",
"accessibility",
]
# This line is edited by the publish_npm_package.yaml workflow. Make sure it is only one line
Copy link
Member Author

Choose a reason for hiding this comment

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

This line was manually added

@hunger
Copy link
Contributor

hunger commented Apr 3, 2025

Let me see whether #8043 works out:

That should fix the issue, too, and should work pretty independent of the format we chose to store the file on disk. It basically reformats with infinite columns, runs the scripts and formats back.

@tronical
Copy link
Member

tronical commented Apr 3, 2025

This seems like a reasonable compromise to me. We still enforce formatting, but it's slightly simpler.

Copy link
Member

@tronical tronical left a comment

Choose a reason for hiding this comment

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

To me this change is an improvement:

  • It's visually more pleasing
  • It adds comments about the automated editing being done
  • It also fixes the packaging

I think #8043 makes sense in parallel, as it makes the automated editing more robust, by itself.

@hunger
Copy link
Contributor

hunger commented Apr 4, 2025

I merged #8043, so the packaging should be fixed, only leaving the more pleasing and the added comment.

As usual I have no strong opinion on the specifics of the configuration of a code formatter: I do not have to worry about formatting stuff myself and fixing spaces in code review.

I would love the settings to move into .taplo.toml though, so people using the taplo LSP (e.g. via the taplo VSCode extension) also have the expected settings:

[formatting]
column_width = 120

should do this trick, but I did not test this.

ogoffart added 2 commits April 9, 2025 12:29
Commit cd6f2e2 reformated the .toml, but the 80 char width column is
judged too small to be practical

Add a .taplo.toml file
@ogoffart
Copy link
Member Author

ogoffart commented Apr 9, 2025

Rebased:

Added a .taplo.toml

I did two commit:
First commit change the max line width,
Second commit change another setting not to split the feature array.

What do you think?

@ogoffart ogoffart merged commit 78a3757 into master Apr 9, 2025
40 checks passed
@ogoffart ogoffart deleted the olivier/toml branch April 9, 2025 13:06
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