fn_width config option

[Sjael]

Thought it'd be nice to be able to choose how long you want function declarations to be before going vertical. This is basically the gate before fn_params_layout kicks in. It used to just be max_width, but now it takes the min between max_width and fn_width.

The default is 100, which is what max_width defaults to so this should not break anything.

[Sjael]

So the checks were spotty there. Then I started to get failed tests half the time on my machine as well after spam rerunning cargo test.

I switched the configs for the test files to toml rather than comment headers and it seems to work consistently now.

I was getting "cant exceed max_width" errors when those test failed. I assume waiting to extract the max_width option from the config comment string is what does it. If another option in that test's config is dependent on that max_width getting read fast (which mine did, as fn_width caps at max_width), it may drop the ball frequently. Test file configs needing max_width could follow this solution if this is indeed the case.

[ytmimi]

@Sjael could you provide a motivating example for why you'd want to wrap before the max_width? I'm a little reluctant about adding this configuration option.

[ytmimi]

The flakey tests around max_width are probably related to #6011. I've got plans to address that one soonish

[Sjael]

Right now I'm working in Bevy and the functions can have many parameters often with small length, which can make them harder to read when all on one line. I would use fn_params_layout but I dont want vertical params on every function I make.

What it looks like now:

fn setup_arena(mut commands: Commands, mut meshes: ResMut<Assets<Mesh>>, icons: Res<Icons>, models: Res<Models>) {

What I'd rather have it look like:

fn setup_arena(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    icons: Res<Icons>,
    models: Res<Models>,
) {

Also, I make my guards a little longer than the default max_width, so if I want single_line_let_else_max_width at 125, that means all my function signatures are even longer because I need to increase max_width to 125. So I thought to just make this options as a sort of slider, not just a binary switch with fn_params_layout = vertical.

[ytmimi]

Definitely not a use case I was thinking of, but it seems reasonable to me. Just a fair warning, I'm trying to go through and review outstanding PRs in the backlog so it might be a while before I can get to this one

[Sjael]

Not a problem at all! Thank you for the quick replies and concise requests for the other PR. It was fun working on it with you.

[ytmimi]

Yeah, it's been fun, and I appreciate you applying all the feedback. I know I still owe you a follow up review on the other PR. I'm hoping to get around to that next week

[ytmimi]

@Sjael thanks for proposing this new option and working on this. Overall this is something the team will need to discuss and consider, but the implementation was relatively straightforward so I went ahead with my initial review.

If you continue to work on this I want to make sure that we're being as thorough as possible with the testing. I've left comments outlining the additional test cases that I want to see. Also, we need to add independent tests, separate from the ones you've already added that show how this option interacts with the various fn_param_layout configurations.

Additional note:
I want to be clear and say that I'm not suggesting we need to create test cases to show how all of these options interact, but I wanted to bring it up for awareness. There's also the space_before_colon, space_after_colon, and type_punctuation_density configuration options that might impact the function declaration width. There may be others, but this is what I found after a quick look over all our configs.

[ytmimi]

let's move these tests to tests/source/config/{config_name}/{value}.rs

[Sjael]

I assume you meant the configs folder, but it throws errors when I put the test files in there so I made a new config folder which is somewhat confusing but no errors.

[Sjael]

I'm thinking about changing this to being a fn_param_limit where the function declaration breaks to new line based on amount of parameters instead of line length.

So

fn lorem(mut commands: Commands, icons: Res<Icons>, mut meshes: ResMut<Assets<Mesh>>) {
    // block
}

would turn into

fn lorem(
    mut commands: Commands,
    icons: Res<Icons>,
    mut meshes: ResMut<Assets<Mesh>>,
) {
    // block
}

when fn_param_limit = 2

The thinking here is that it's not really the length of the line that makes it harder to read, it's when there are too many parameters on one line.

[ytmimi]

I'm thinking about changing this to being a fn_param_limit where the function declaration breaks to new line based on amount of parameters instead of line length.

I'll need some time to think about that idea. My gut reaction is to stick with the line length wrapping. To the best of my knowledge rustfmts wrapping configurations all revolve around line length so choosing a number of items seems inconsistent. For now I'd like to keep this focused on length as apposed to the number of items.

[Sjael]

Okay I'll change it back to length. I understand that this makes sense from the standpoint of staying consistent with 'length' being the primary limit for wrapping to a new line. I am wondering though, do you agree/disagree with this part of my previous comment?

The thinking here is that it's not really the length of the line that makes it harder to read, it's when there are too many parameters on one line.

Perhaps the solution isn't what is needed, but would you agree that the number of arguments is what can lead to an illegible line, rather than specifically the length?

[ytmimi]

Okay I'll change it back to length. I understand that this makes sense from the standpoint of staying consistent with 'length' being the primary limit for wrapping to a new line. I am wondering though, do you agree/disagree with this part of my previous comment?

The thinking here is that it's not really the length of the line that makes it harder to read, it's when there are too many parameters on one line.

Perhaps the solution isn't what is needed, but would you agree that the number of arguments is what can lead to an illegible line, rather than specifically the length?

Yes, and no. I can definitely see why in some cases having too many arguments on one line might make it more difficult, and I can also see how keeping function signature length tied to the max_width would only exacerbate the issue, but I don't think number of arguments alone is what causes poor readability.

The complexity of the types and whether or not you're pattern matching on any of the arguments could be another source of poor readability for some users.