-
Notifications
You must be signed in to change notification settings - Fork 127
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 taplo
CI + format all TOML files
#915
Conversation
Bencher
🚨 10 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
523a16e
to
d64ed78
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
e3f62da
to
dcd231f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
b25e883
to
8fe27ce
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really convinced about formatting Cargo.toml
files. As long as they are not huge, the normal formatting is just fine. whats the motivation for including them here?
original motivation was just to bring some uniformity to them... there's random patterns accross multiple files but also, if we include taplo on CI, we need all TOML files to conform to those rules, otherwise CI will break for every PR |
I think if you are eager to format Cant |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
4d20fed
to
7086871
Compare
This comment was marked as outdated.
This comment was marked as outdated.
You can exclude Edit: My main concern here is that we are using the same CI process + same formatting setup for a config intended for users and a config(Cargo.toml) we use, requirements between those two could diverge easily |
7c4455f
to
39b4218
Compare
I rebased. Now we have three separate They don't have any specific formatting policies however, only @jbesraa do you have any specific ideas on formatting policies, or did you just want to make this PR "future-proof" in that sense? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New approach looks good.
In the future it would be good to move all the configs to a folder in root, not sure they should live in the crate itself
serde_json = { version = "1.0.64", default-features = false, features = ["alloc"] } | ||
tracing = {version = "0.1"} | ||
binary_sv2 = { version = "^1.0.0", path = "../v2/binary-sv2/binary-sv2"} | ||
serde = { version = "1.0.89", default-features = false, features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line break because of line length? the previous formatting made more sense(each crate on its own line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, the .taplo*.toml
files are all empty with regards to actual formatting policies, they only do filtering for which file they will format
so I assume this is coming from the default formatting policy that taplo
brings
91dfedf
to
b8d0034
Compare
I'm reverting this back to Draft status merging this will have an undesired impact on our semver checking on CI, see this comment for more details: #985 (comment) we should get #985 sorted out before merging this @pavlenex we can remove this from milestone 1.0.2, since #985 could take a while and we don't want the next release blocked |
Maybe someday. |
close #914 by:
taplo