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(forge): fmt multiline function header style config #3252

Merged
merged 5 commits into from
Sep 18, 2022

Conversation

rkrasiuk
Copy link
Collaborator

@rkrasiuk rkrasiuk commented Sep 17, 2022

Motivation

close #3186

Solution

This PR contains breaking change of removing func_attrs_with_params_multiline config option in favor of multiline_func_header style option. This does change the default behavior of the formatter. Keeping as a draft for a discussion

@mds1 @gakonst @transmissions11

@rkrasiuk rkrasiuk added T-feature Type: feature C-forge Command: forge Cmd-forge-fmt Command: forge fmt labels Sep 17, 2022
@rkrasiuk rkrasiuk marked this pull request as draft September 17, 2022 13:48
@mds1
Copy link
Collaborator

mds1 commented Sep 18, 2022

From the source:

pub enum MultilineFuncHeaderStyle {
    /// Write function parameters multiline first
    ParamsFirst,
    /// Write function attributes multiline first
    AttributesFirst,
    /// If function params or attrs are multiline
    /// split the rest
    All,
}

So IIUC, ParamsFirst does this:

function computeAddresses(
  address asset
) public view returns (address, address) {

AttributesFirst does this:

function computeAddresses(address asset)
  public
  view
  returns (address, address)
{

But:

  1. I'm not sure I understand what All does
  2. Does ParamsFirst imply wrapping return params too, like the below? And if so does when are inputs vs. outputs split?
function foo(address asset) public view returns (
  address,
  address,
  address,
  address
) {

@rkrasiuk
Copy link
Collaborator Author

rkrasiuk commented Sep 18, 2022

@mds1

  1. All means if one is multiline, write the other multiline as well
  2. the returns keyword is really treated as modifier considering its location in function definition. the formatter will try to fit return params in one line or split multiline regardless whether modifier are on single line or multiline

@mds1
Copy link
Collaborator

mds1 commented Sep 18, 2022

Got it, this approach sgtm then!

@transmissions11
Copy link
Contributor

this looks great!

@rkrasiuk rkrasiuk marked this pull request as ready for review September 18, 2022 19:08
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.

lgtm,

love the testing we have for fmt

@mattsse mattsse merged commit 6c9aec3 into master Sep 18, 2022
@mattsse mattsse deleted the rkrasiuk/feat-fmt-param-multiline-config branch September 18, 2022 20:02
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
)

* param multiline config

* remove func_attrs_with_params_multiline

* clippy

* fmt testdata
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-fmt Command: forge fmt T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat (fmt): allow params to break over multiple lines instead of modifiers
4 participants