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

80 character limit #142

Closed
jrobertson-insite opened this issue Apr 24, 2021 · 8 comments
Closed

80 character limit #142

jrobertson-insite opened this issue Apr 24, 2021 · 8 comments

Comments

@jrobertson-insite
Copy link

Why does csharpier use the ancient 80 character limit per line? I realize this is a contentious (religious) issue and there is not "right" answer, but 80 is pretty narrow. In particular as 2560x1440 monitors become more ubiquitous and most people using 2 or more monitors. With those things in mind the popular side by side code windows argument seems less compelling.

Many of the uglier outputs I see from csharpier are from forced line breaks at 80 characters that would look fine at a more reasonable 120 characters.

See examples (I assume there are more in the issues that I haven't come across yet)
#90 #100 #133

@shocklateboy92
Copy link
Collaborator

shocklateboy92 commented Apr 24, 2021

I agree that 80 is a bit wasteful in modern development environments. It's not often you need to look at 3 files at once 😁

Many of the uglier outputs I see from csharpier are from forced line breaks at 80 characters that would look fine at a more reasonable 120 characters.

Those won't really be fixed by increasing the line width - the symbol names could be longer or they could start at a greater level of indent because they're nested in something. We have to figure out our breaking strategies for all syntaxes. If anything, formatting our own code with such a small character limit exposed a lot of areas where our wrapping logic is sub-par.

With those things in mind the popular side by side code windows argument seems less compelling.

Side by side code windows was never a compelling argument. The reason for character limit was always code review. It allows people to do side by side diff, which (once you get used to it) allows you to parse the changes much quicker in your brain.

In conclusion: if/when we eventually get around to allowing things to be configurable, this makes sense to be an option.
However, in the meantime, how about we pick 100 as a sensible default?

@belav
Copy link
Owner

belav commented Apr 25, 2021

Those won't really be fixed by increasing the line width - the symbol names could be longer or they could start at a greater level of indent because they're nested in something. We have to figure out our breaking strategies for all syntaxes.

I completely agree with this.

I think I'm on board with switching CSharpier to default to 100 for now. We can always leave our unit tests at 80, or make them configurable to make it easier to test breaking strategies. The option for line width is there in code, it just isn't exposed yet to the CLI.

For reference - these links explain the defaults for prettier(80) and black(88)
https://prettier.io/docs/en/options.html
https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

@respel
Copy link

respel commented May 12, 2021

image

This is a really interesting experiment suggested on the black docs. It can be done on the csharpier-repos thing you guys have. It could be a really good argument to bump or not bump up the character limit.

@dishuk13
Copy link
Contributor

dishuk13 commented May 13, 2021

image

So, I went ahead and did a little experiment myself. A little caveat is that I didn't ignore the files ignored by default by csharpier during the calculation(.csharpierignore was still taken into account).

Yes, the benefits of reduction in line length aren't linear as we increase print width. But contrary to the black findings, I didn't find any sudden drop anywhere.

Edit: "Avg. line length" is supposed to be "Avg. file length" in the graph and text above.

@belav
Copy link
Owner

belav commented May 13, 2021

I think this is an excellent idea! Because the formatting is still changing the results could change over time so it probably makes sense to run it automatically and report numbers, or to hold off on experimenting too much until the formatting is more stable. Then before going into beta we can determine if the default printWidth should be something other than 100.

@divyanshshukla13 Did you perhaps mislabel the vertical axis on that graph? I was wondering how average line length could be bigger as print width is shorter, but then realized maybe you had meant to label it "Avg file length".

@dishuk13
Copy link
Contributor

Did you perhaps mislabel the vertical axis on that graph?

Yeah, my bad. It's supposed to be "avg. file length".

@Sti2nd
Copy link

Sti2nd commented Jul 28, 2021

I might add to this discussion that short lines (of plain text) are considered faster to read, so by increasing the lenght one are actually making the code harder to read... 🙃

Anyway, I think this issue is the complete opposite of the philosophy of Prettier? @jrobertson-insite says

Why does csharpier use the ancient 80 character limit per line?

The keyword here is ancient. The screen size will probably change from time to time and I (and Prettier) argue the preferred character limit is just another opinion. We can imagine that in 10 years the monitors will be twice the size again, or maybe we have all switched to iPads and actually prefer shorter lines than 100 characters. Getting rid of discussions about style is a part of the Prettier philosophy and one of the huge benefits of using it!

For this specific issue, if you think 100 is more future proof than 80, I say go with it. Just remember that the whole point of a (Prettier) auto-formatter implies the print width should not change!

Side note: I am not sure how csharpier does it, but at Prettier it is documented that 80 is just a guideline and not a limit. It tries to make it readable :)

You are doing a great, job @belav 🏆 🏅 !! Looking forward to use csharpier in all my professional projects when it goes stable 😍

@belav
Copy link
Owner

belav commented Aug 17, 2021

For this specific issue, if you think 100 is more future proof than 80, I say go with it. Just remember that the whole point of a (Prettier) auto-formatter implies the print width should not change!

I just kind of picked 100 because 80 did feel too small in c# and 100 felt better to me. Note that print width is the only thing configurable in CSharpier right now. There were a few other options but they have gone away.

After some thought, I think I know why 100 feels about right.
As of now, almost every c# file you look at is something like this

namespace Namespace
{
    public class Class
    {
        // anything in a class is effectively a print width of 92
        public void Method()
        {
            // and anything in a method is effectively 88
        }
    }
}

If the default was 80, then code in methods would effectively be 68, which is pretty short.
Two spaces for tabs would help, but I never see c# written that way.

C# 10 is introducing file scoped namespaces which will affect my numbers - dotnet/csharplang#137

Because it is configurable the default is probably going to stay at 100. Although it is worth considering removing the option to avoid the inevitable debates about what to set it to.... hmmmmm

You are doing a great, job @belav 🏆 🏅 !! Looking forward to use csharpier in all my professional projects when it goes stable 😍

Thank you! I feel like it is getting very close now.

@belav belav closed this as completed Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants