refactor(fmt): change default for override_spacing to false

[PaulRBerg]

Motivation

The Solidity documentation uses no spacing between the override keyword and the overridden contract names:

contract Inherited is Base1, Base2
{
    // Derives from multiple bases defining foo(), so we must explicitly
    // override it
    function foo() public override(Base1, Base2) {}
}

Anecdotally, most Solidity code bases I've seen adhere to the same pattern where there is no spacing. This is perhaps the same reason why @benesjan opened issue #3522 in the first place.

Solution

Set the default value of variable_override_spacing to false.

Though, before this is merged in, we should fix #4025 first.

[gakonst]

what happens when there's no override target? i.e. it's just override because you have is Base1

[PaulRBerg]

what happens when there's no override target?

I wanted to test this but unfortunately the newest override_spacing doesn't work anymore - setting to false doesn't remove the spacing now. This started happening immediately after upgrading to the latest version of Forge:

forge 0.2.0 (a44159a 2023-01-03T00:04:41.810446Z)

Cc-ing @rkrasiuk in case he can take a look?

[rkrasiuk]

@PaulRBerg ugh, must be some config issue, looking

[rkrasiuk]

@PaulRBerg the issue is unrelated, couple of latest releases failed
https://github.com/foundry-rs/foundry/actions/runs/3851356109

[PaulRBerg]

Thanks, Roman, I will wait for the fix and try test again with only one base contract inherited.

[rkrasiuk]

@PaulRBerg openssl got pulled into foundry again 😢 . we added a fix and CI check to prevent this from happening again. will re-release as soon as the fix is merged and ping you

[gakonst]

what do we think of this?

[rkrasiuk]

@gakonst in general, some tests need to be fixed with this change to mirror the default behavior. the exact issue on the screenshot is due to bad rendering. here's this branch on cli

[rkrasiuk]

@PaulRBerg there are some tests that need to be fixed to depict the changed default behavior. there are 3 override-spacing.fmt.sol files in testdata. the easiest way forward is to rename these to fmt.sol files and remove // config: override_spacing = false line (since this is now the default). the original fmt.sol files should be renamed to override-spacing.fmt.sol and should include // config: override_spacing = true at the top

[rkrasiuk]

@PaulRBerg just checked, override_spacing option works on latest release 🫡

[PaulRBerg]

Thanks for the quick turnaround, @rkrasiuk! I can confirm that override_spacing works in the latest release.

Re tests - thanks for the guidance! I have updated them to match the new default.

[rkrasiuk]

lgtm, will merge after checks pass

[rkrasiuk]

@PaulRBerg seems like some other files with default behavior need to be fixed as well

[PaulRBerg]

@rkrasiuk I took a look at the CI logs but I don't know what to make out of them .. I don't understand what went wrong. Looks like the "" formatting is the same as the ""?

[rkrasiuk]

@PaulRBerg the CI logs do not show the space diff for some reason (see my response to @gakonst above). fixing the tests

[PaulRBerg]

Oh, I see. Sorry for the back-and-forth, and thanks for the fix.