-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix/CICD ~ cleanup and refactor with multiple associated fixes #4974
Conversation
All checks/tests are now passing. |
nanos += 1_000_000_000; | ||
seconds -= 1; | ||
} | ||
if let Ok(offset) = parse_datetime::from_str(date) { |
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.
please move this change into a different PR
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 can, but the point was to get "green" and stop all the failed CI builds.
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.
It doesn't really matter IMHO
@@ -0,0 +1,7 @@ | |||
# `taplo` configuration |
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 would rather keep the defaults of taplo
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.
It splits many of the dependency lines in the Cargo.toml
files making it harder to keep them organized.
And it pushes around end-of-line comments to ridiculous locations.
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.
Do you have examples of that?
For example, this one seems unrelated:
array_auto_collapse = false
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 added them iteratively as formatting problems cropped up.
I don't remember the details for the need for that line.
It might have been needed before the final format was completed, and redundant now.
I'll remove that line it locally and see if anything crops back up.
...
I found the issue. Without that configuration, taplo
collapses all the feature sets (eg, feat_common_core
, feat_Tier1
, etc) into single lines, making them harder to read and curate.
Basically, some TOML arrays are more readable/useful collapsed and some are better expanded.
Just collapsing (or expanding) all of them removes the textual clues that help reduce cognitive load when reading through and updating the code.
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.
All of the configuration tags here are purposeful and help me keep the TOML organized and readable.
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.
ok, i don't like it but i won't spend my time arguing about coding style. I just wish you didn't change the default...
anyway, if you want to change them, please do it a separate PR
deny.toml
Outdated
@@ -27,7 +27,7 @@ allow = [ | |||
"BSD-2-Clause-FreeBSD", | |||
"BSD-3-Clause", | |||
"CC0-1.0", | |||
"MPL-2.0", # XXX considered copyleft? | |||
# "MPL-2.0", # * unused # ToDO?: considered copyleft? remove? |
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.
please just remove it :)
I'm rechecking the Android CI step; it looks like it timed-out waiting for the emulator. |
name: toybox-result.json | ||
path: ${{ steps.vars.outputs.TEST_SUMMARY_FILE }} | ||
|
||
toml_format: |
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.
please put it back in its own tasks. It makes reviews easier
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.
Could you please split this PR into smaller?
it is a mixing way too many different things in the same PR.
I don't mind having individual PR being red
Thanks
smallvec = { version = "1.10", features = ["union"] } | ||
tempfile = "3.6.0" | ||
term_grid = "0.1.5" | ||
terminal_size = "0.2.6" | ||
textwrap = { version = "0.16.0", features = ["terminal_size"] } | ||
thiserror = "1.0" | ||
time = { version = "0.3" } | ||
time = { version = "=0.3.20" } ## maint: [2023-06-11; rivy] pinned to maintain MinSRV 1.64.0 |
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.
@sylvestre , this end-of-line comment gets blown out to starting at 100+ character position without the align_comments = false
.
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.
you can probably make the comment shorter. remove the "maint" and the "maintain"
or move it the line above
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.
IMO, it's better located on the line that it refers to so that they don't get separated with future change.
Ultimately, it'll be removable (and should be removed along with the =
pin) when MinSRV increases.
And, no matter the length, if it's on the same line, taplo
is pushing it to the right side, with an extremely wide area of white space between it and the code it refers too.
The Android CI tests are consistently failing on the emulator setup. |
Sure, I'll try to refine it and split it. I get that a failed CI isn't the worst of issues, but it becomes chronic. And then things start slipping through that aren't seen because we think we know why it's failing. It becomes much more messy to fix it. |
7f617c6
to
b3afd73
Compare
2ccc524
to
907641c
Compare
most of the changes have been merged: closing |
The plan is for this commit set to get CI to back to green (✔️).
I'll convert from draft when the set builds/tests cleanly.