Random chores

[chitoyuu]

Performed a number of miscellaneous chores around the codebase. This PR contains a number of unrelated changes: please check the commit log for details. A list of all issues created in the process can be found in #377.

Close #377.

[chitoyuu]

YAML, not even once.

bors try

[bors]

try

Build succeeded:

• full-ci

[chitoyuu]

Updated all invocations of format!-like macros to the new inline syntax with help from cargo clippy --fix. This new syntax was introduced in Rust 1.58, which is below our current MSRV of 1.63.

Honestly, I still find it a bit weird to look at, but rationally I also know that this is how interpolated strings work in many other languages, and what really should have been supported all along. I just guess it's going to take a while to get used to.

Re-running full CI just to be sure.

bors try

[chitoyuu]

I swear I reverted those changes to the casts, but somehow they still got into the commit, so...

bors try

[chitoyuu]

bors try

[bors]

try

Build succeeded:

• full-ci

[chitoyuu]

Rebased on master and clippy should no longer be complaining on nightly.

[Bromeon]

      - name: "Look for TODO comments without issue numbers attached to them"

This is a fantastic idea! Once GDExtension becomes a bit more stable, I might want to steal this 😉

I guess the reason to include it in minimal-ci as opposed to full-ci was that it's very short-running and one gets immediate feedback?

[chitoyuu]

I guess the reason to include it in minimal-ci as opposed to full-ci was that it's very short-running and one gets immediate feedback?

Yes, that is the intention. Putting this in full-ci creates a bottleneck -- a stray TODO that went through the cracks isn't the best way to stop an r+.

[chitoyuu]

Expanded the todo check into a self-testing shell script, which should hopefully allow us more confidence while working with it.

bors r+

[bors]

Build succeeded:

• full-ci

[Bromeon]

Updated all invocations of format!-like macros to the new inline syntax with help from cargo clippy --fix. This new syntax was introduced in Rust 1.58, which is below our current MSRV of 1.63.

Honestly, I still find it a bit weird to look at, but rationally I also know that this is how interpolated strings work in many other languages, and what really should have been supported all along. I just guess it's going to take a while to get used to.

On that note, Rust 1.67.1 (a patch release!) reverted their decision to warn by default on clippy::uninlined_format_args.

Old-style format strings will now be accepted again by CI, unless we explicitly enable that lint.

[chitoyuu]

That's very nice to know. Do you think we should enable that lint manually then? The patch notes said that it's supposed to be a temporary workaround, and it sounds like that they intend to revert it pretty soon:

Additionally, the clippy style lint uninlined_format_args is temporarily downgraded to pedantic -- allowed by default. ...rust-analyzer does not support it yet

We can also just wait for the reversion, but then we risk having to mass-fix again depending on how soon "soon" is going to be.

[Bromeon]

One issue with the lint is that you can get into a jittery state, forcing you to change style all the time:

fn print(v: Vector3) -> String {
    format!("({}, {}, {})", v.x, v.y, v.z)
}

// -- refactor -->
fn do_sth(v: Vector3) -> String {
    let Vector3 { x, y, z } = v;
    format!("({}, {}, {})", x, y, z) // ERROR
}

// -- satisfy clippy -->
fn do_sth(v: Vector3) -> String {
    let Vector3 { x, y, z } = v;
    format!("({x}, {y}, {z})")
}

// -- refactor again: encapsulate fields -->
fn do_sth(v: Vector3) -> String {
    // now needs to change again
    format!("({}, {}, {})", v.x(), v.y(), v.z())
}

Or worse, when some format arguments are inlineable and others aren't. But that probably happens rarely enough that it's worth the cases where it does increase readability.

Yeah, maybe we can manually enforce the lint as a bridge, until it's re-enabled in clippy. Assuming we actually agree with the idea behind it -- it is one of the more opinionated ones, and I wouldn't call the old format strings "bad style".

[chitoyuu]

Good point. Personally I don't have any strong feelings either way, as long as we stick to a decision and do not perform repo-wide changes all the time. For refactors, it isn't really "extra work" to change the format string too when the rest of the line is changing anyway.

A lot of the things we're formatting in gdnative are error messages, constituting of large chunks of plain text and a few local variables interspersed in. For those, the new syntax is indeed a lot more readable, and in fact easier to refactor. This one from the diff for example:

// before
write!(f, "cannot convert argument `{}` (#{}, {:?}) to {}: {} (non-primitive types may impose structural checks)", name, idx, value, ty, err)
// after
write!(f, "cannot convert argument `{name}` (#{idx}, {value:?}) to {ty}: {err} (non-primitive types may impose structural checks)")

It's immediately obvious with the new syntax what the placeholders are actually for, and very easy to reorder the text or insert a new field in the middle, where with the old syntax it would require a lot of counting.

[Bromeon]

I'm also fine with both enforcing this and waiting until clippy re-introduces the lint. But you're right that if we choose the latter, it might require a larger fixing. Should it then be enforced?

Another change I planned to make was to verify intra-doc links. I likely won't have time until middle next week though, but if you want, I might test this in GDExtension and later open a PR here too; then I could also include the Clippy lint. Or we do the two things independently, also fine 🙂