Skip to content
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

feat(fmt): add all_params config - same as all but split single param too #9176

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

grandizzy
Copy link
Collaborator

Motivation

Closes #9173

  • for backward compatibility reasons multiline_func_header="all fmt does not write single params on new line (at param level there's params_first which writes on new line regardless number of params and params_first_multi which writes only multi params on new lines)
  • add a new config multiline_func_header="all_params to write params on new lines even if there's a single param (adding such won't break existing formatters)

Solution

@grandizzy grandizzy changed the title fet(fmt): add all_params config - smae as all but split single param too fet(fmt): add all_params config - same as all but split single param too Oct 23, 2024
@grandizzy grandizzy marked this pull request as ready for review October 23, 2024 14:50
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great!

@grandizzy grandizzy changed the title fet(fmt): add all_params config - same as all but split single param too feat(fmt): add all_params config - same as all but split single param too Oct 23, 2024
@grandizzy grandizzy merged commit b1e9365 into foundry-rs:master Oct 23, 2024
21 checks passed
@grandizzy grandizzy deleted the issue-9173 branch October 23, 2024 17:37
@ChiTimesChi
Copy link
Contributor

LFG! Thanks :)

Comment on lines +6 to +8
function oneParam(
uint256 x
);
Copy link
Contributor

Choose a reason for hiding this comment

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

@grandizzy I had a chance to test this today, and I see that this doesn't match the previous "multiline_func_header = all" behaviour :(

I apologise for the confusion, but the behaviour I'm trying to recreate is the following:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I am at a lost here, in the issue you created you mention that

function a(uint256 param1)

it's a bug and should be expected to be as

function a(
    uint256 param1
)

which the change from this PR is doing, but actually it shouldn't be multilined now?
Please add relevant case expected vs actual and will look into, a diff would also help

Copy link
Contributor

Choose a reason for hiding this comment

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

@grandizzy Here's the relevant test case:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.20;

contract A {
    // Single parameter is printed on a new line, when the function definition is multi-line
    function a(
        uint256 param1
    )
        external
        pure
        returns (uint256 firstReturnValue, uint256 secondReturnValue, uint256 thirdReturnValue)
    {
        firstReturnValue = param1 + 1;
        secondReturnValue = param1 + 2;
        thirdReturnValue = param1 + 3;
    }

    // Single parameter is printed on the same line, when the function definition is single-line
    function b(uint256 param1) external pure returns (uint256) {
        return param1 + 1;
    }
}

I think I got this working in my local foundry fork, do you mind if try opening a feature PR for that? Assuming it would be fine to modify the newly added AllParams option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, sure, please do a PR, AllParams was added to accommodate those diffs so all good. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, I am trying to replicate the multiline_func_header=all behaviour from the August build we were using. The set of formatting rules made sense at the time, and still does :)

rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
…aram too (foundry-rs#9176)

fet(fmt): add all_params config - smae as all but split single param too
@grandizzy grandizzy added T-feature Type: feature C-forge Command: forge labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

feat(fmt): multiline_func_header all option to multiline also single function parameter
3 participants