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

Pub should not be offended by packages that aren't using dartfmt with line length 80. #3956

Closed
Hixie opened this issue Aug 20, 2020 · 25 comments

Comments

@Hixie
Copy link

Hixie commented Aug 20, 2020

According to the discussion around flutter/flutter#51752 (comment), we penalize (either literally in the score or maybe just with an informational message) packages that don't use dartfmt with line length 80. This is forcing people to write bad code (as discussed in that bug, for example, there are people literally changing how they write build methods to stay within 80 characters).

@esDotDev
Copy link

esDotDev commented Aug 20, 2020

Example package here:
https://pub.dev/packages/sized_context/score

image

@sigurdm sigurdm transferred this issue from dart-lang/pub Aug 21, 2020
@sigurdm
Copy link
Contributor

sigurdm commented Aug 21, 2020

cc @mit-mit @munificent @jonasfj I guess there are some opinions to be had here.

We could stop checking for conformity with dartfmt.
I believe on the whole enforcing dartfmt is doing more good than bad for code quality (and certainly for productivity) - but I agree that the strict 80 character limit feels constricting sometimes...

Another suggestion would be to have a dartfmt_options.yaml file indicating the prefered line length for the project, but I think that has been proposed several times and not done for reasons of keeping things simple, uniform and easy to use: https://github.com/dart-lang/dart_style/wiki/FAQ#why-cant-i-configure-it.

@Hixie
Copy link
Author

Hixie commented Aug 21, 2020

80 characters is just too short for any reasonable Flutter code, for what it's worth.

@Hixie
Copy link
Author

Hixie commented Aug 21, 2020

(I'm struggling to see how enforcing a formatting on pub package authors does anything but reduce the number of people who would submit code. I know that it would certainly cause me to stop submitting packages. I don't know if we are trying to discourage people like me from submitting packages, maybe that's working as intended...)

@isoos
Copy link
Collaborator

isoos commented Aug 21, 2020

Pub should not be offended by packages that aren't using dartfmt with line length 80.

The uploaded package is getting published, it is getting analyzed, distributed, advertized in search... The only thing it misses in such cases is the max score. I wouldn't classify it as "offended" or encouraging "bad code".

I think it is reasonable to indicate to the potential users of the package what they can expect from it. Such indication should include how the code is formatted, because if they need to touch it for any reason, they are slightly ahead if it follows the dartfmt defaults (not every developer uses wide screens with 120 character lines). In case there are two similar packages available for the same function, I'd probably choose the one which has better match with the community standard and expectations.

Points are easy to grasp, and not getting all the points get people offended. We could switch out the points to another indication (badges, stars, banners), but it will boil down to the same gamification impulses: developers who are not following the community standards but still want to get "max score" will get angry. The only confrontation-aversion thing is to dilute these checks against standards, lowering their usefulness and signal value.

there are people literally changing how they write build methods to stay within 80 characters

I think writing the referenced example as two method calls is just as reasonable and readable, and I wouldn't classify it as "bad code".

object.method();
object.method2();

We have many places (e.g. optional , at the end of the arguments list) that affect the formatting and with that the readability of the code. Whether the code author uses them is up to them - it is their choice.

Having said that, if the tool's defaults do not serve the majority of the developers, we should change the defaults in the tool. Either by changing the recommendation, or allowing the tool to accept multiple line lengths at the same time.

pub could technically run check against 80, 81, 82, ... 200-length lines each, and if the code conforms any, we accept it. However, that has very little value of communicating to the users of the package what they can expect from it.

Another option would be to introduce yet another config file to declare that the package follows whatever line line of choice the author has (let's say it is 120), and pub checks if it conforms to that setting and award points/badges if they do. But then pub should also communicate that setting on the package page.

@sigurdm
Copy link
Contributor

sigurdm commented Aug 21, 2020

I'm struggling to see how enforcing a formatting on pub package authors does anything but reduce the number of people who would submit code. I know that it would certainly cause me to stop submitting packages.

It is supposed to encourage having well-formatted code obtained easily and mechanically as that makes contributing to the projects easier.

To give a counterpoint - back when I was on the flutter team I always dreaded hand-formatting code to reach flutter-standards. Not that the standards are bad - it is just a lot of work to manually format code, and mistakes easily slip through.

I don't know if we are trying to discourage people like me from submitting packages, maybe that's working as intended...)

That's absolutely not the intention!

I would also like dartfmt to be more lenient on line-length, but I know it saves me countless hours of boring manual labor, and it makes it trivial for me to correctly format a contribution to a third-party project correctly.

@mit-mit
Copy link
Member

mit-mit commented Aug 21, 2020

Pub should not be offended by packages that aren't using dartfmt with line length 80

we penalize (either literally in the score or maybe just with an informational message)

Pub is not offended. Pub doesn't penalize. Pub scores packages on an increasing scale; those that follow Dart best practices are awarded additional points than those that don't. There is no penalty. There is just less reward.

80 characters is just too short for any reasonable Flutter code, for what it's worth.

80 characters is the dartfmt standard; a standard that goes back many, many years. 80 chars also happens to be a very common line length across the industry.

We could debate what the optimal line width is. This is likely to be a highly subjective debate, and thus in direct opposition to the goal of automated formatting: Establish a set of common criteria so that we don't have to waste hours of code review time on subjective preferences.

Further, any time we deviate from such standards, we risk having issues with things like standard tools. Here are a few examples:

  1. I can't read all of the code in comment Reusing state logic is either too verbose or too difficult flutter/flutter#51752 (comment) without using the scroll bar as it's too wide

Screenshot 2020-08-21 at 15 35 06

  1. I can't see the end of the line _lerpProperties<T> in https://github.com/flutter/flutter/pull/64316/files#diff-7d9595fad09aaa3c123e41f18f6cece0R226 in the github code review view, even if I make the browser full width on a 27" screen. I can go hunting for the horizontal scroll bar, but that is far from the screen location that has the long line, and thus cumbersome.

Screenshot 2020-08-21 at 15 40 55

@Hixie
Copy link
Author

Hixie commented Aug 21, 2020

There is no penalty. There is just less reward.

I think in practice (as seen in the referenced bug) this is a distinction without a difference.

It is supposed to encourage having well-formatted code obtained easily and mechanically as that makes contributing to the projects easier.

I think that makes sense for projects that want contributors, but are we saying we don't want people to publish packages that they are only interested in working on themselves?

@esDotDev
Copy link

esDotDev commented Aug 24, 2020

I think it is reasonable to indicate to the potential users of the package what they can expect from it.

This feels like a bit of a stretch regarding line length. At least in the Flutter eco-system, plugins are not edited and it's discouraged by the IDE. I'm not sure why a user would care so much about line breaks for code they will only be reading.

There is no penalty. There is just less reward.

This is just a semantic difference from the dev POV. We want to maximize the score in order to maximize usage for the package, we could care less these pts are called, but we will jump through any hoops in order to maximize the score. It's not really considered optional, we don't look at these as "bonus pts", anything < perfect is always viewed as a penalty.

From a pragmatic standpoint, hopefully rolling out a penalty like this out of the blue is not going to be a common practice. It's kinda frustrating to come back months later, and find out 5 packages you published are all now down-ranked 10 pts creating a bunch of make-work for you.

@isoos
Copy link
Collaborator

isoos commented Aug 24, 2020

From a pragmatic standpoint, hopefully rolling out a penalty like this out of the blue is not going to be a common practice.

FWIW pub.dev had the dartfmt check from the first days since we've had package analysis, and points were deducted when the source code was not matching the then-current version of darfmt. For a long time the "health score penalty" was 1 - 0.995^(number of files not well formatted), which, if you had more than 21 non-formatted files was more points lost than today's fixed 10%. The old penalty was not that front-and-center, because the displayed score on the listing page was a weighted average of health, maintenance and popularity, but it was always there (with some limit in the past 8-12 month IIRC).

We want to maximize the score in order to maximize usage for the package, we could care less these pts are called, but we will jump through any hoops in order to maximize the score.

Maximum score is awarded to packages that follow the community standards / tool defaults.

@munificent
Copy link
Member

80 characters is just too short for any reasonable Flutter code, for what it's worth.

All of the Flutter code inside Google fits within that without undue pain.

@Hixie
Copy link
Author

Hixie commented Aug 25, 2020

That's not been my experience, but YMMV.

@meritozh
Copy link

meritozh commented Nov 3, 2020

Any reason force people use 80? In my 27' display, 80 is ugly, too many blank. I agree it should suggest people format these code, but not one specific code style.

@Klerith
Copy link

Klerith commented Dec 5, 2020

Same issue here, I pass the 80 character length because I'm adding asserts in my constructors, but after the dartfmt -w . stays the same.

Screen Shot 2020-12-05 at 4 03 19 PM

@munificent
Copy link
Member

I'm adding asserts in my constructors, but after the dartfmt -w . stays the same.

Would you mind filing an issue here for that? The formatter should be wrapping those to fit the column limit.

@fzyzcjy
Copy link

fzyzcjy commented Oct 6, 2021

Any updates after a year? This is quite helpful, thanks!

@mzimmerm
Copy link

As stated on other places, why not to allow projects to specify project's default line-length in analysis_options.yaml or pubspec.yaml?

@munificent
Copy link
Member

As stated on other places, why not to allow projects to specify project's default line-length in analysis_options.yaml or pubspec.yaml?

Dart format does not have any notion of "project" or "package" and doesn't know where to look for any sort of global or package-level configuration data. It just formats each file path you give in a completely context-free manner.

This is important because:

  1. It's faster. The formatter doesn't look through parent directories for config files and determine which package a given file belongs to before it can determine how to format it.
  2. It ensures you get consistent formatting when formatting files on disk or in your editor. A lot of editors invoke dart format by passing a string of code to it on stdin and reading the result back from stdout. For those invocations, there is no file path that the formatter could use to find the config file. We could require IDEs to pass in the desired line length, but that's likely to be brittle.

We have millions and millons of lines of Dart code inside Google formatted to 80 columns. I can absolutely promise you that Googlers have come up with class names and method names as long as or longer than anything you see in the wild. This is nowhere near the top of the priority list for improving Googlers' lives and I find it very hard to believe that configurable line length will measurably improve the lives of any external users either.

I have never seen code that I felt would be easier to read with a longer line length. I've seen a lot of code that would be easier to read if the author rethought the verbosity of some of their identifiers.

@esDotDev
Copy link

esDotDev commented Jan 12, 2022

This is highly dependent on your environment.

In pure dart code, starting from an indentation of 2 or 4 spaces, 80 chars is probably a reasonable length, though still does not take very good advantage of larger monitor sizes, where I have loads of horizontal space, and limited vertical space. 80 chars looks comical on an ultra-wide, while it is perfect on a laptop.

But if you're writing flutter code, it's a different story:

  • Flutter uses an extreme amount of indentation and nesting, it's not uncommon to start a line 16 or 20 chars in the hole. So now you are really talking about a 60 char line length, which is stifling.
  • Flutter also uses fairly verbose parameter and class names
  • The combination of those two things, + 80 char break point, can leads to a very high line count, which makes vertical space a somewhat precious resource and can really reduce the readability of the entire class, essentially obfuscating the more important code.

In Flutter the sweet spot is really much closer to 100-120.

It's also not about necessarily reading any one line in isolation, but being able to grok a large group of lines easily. When you optimize only for the readability of an individual statement, you may negatively impact the readability of the entire file. For example, if your Flutter class has only 10 lines of important / unique code, it will generally be easier to find that code and reason about it's behavior, if the surrounding boilerplate of flutter widgets and layout, could minimize their vertical footprint as much as possible. Essentially I find Flutter has a signal:noise ratio problem, and being able to strategically collapse certain "unimporant" lines can really help reduce the noise.

I realize these are all implementation details of Flutter, that are not really dart specific, but it's also reality for most dart developers.

@isoos
Copy link
Collaborator

isoos commented Jan 12, 2022

package:pana (and analysis on pub.dev) gives higher scores to packages that match the default and/or the recommended standards, helping the overall community to consume these packages in a uniform way. If/when the dart format tool accepts different line lengths, analysis on pub.dev will follow transparently. If you have specific suggestions on how a tool should change its default behavior or what recommendation we put out, it should be raised in that repository, and not here.

Personally, I have found that extracting deep structures into private builder methods and Widgets helped a lot to make my Flutter code readable. It is possible that it could have be done with longer line length and larger monitor/resolution, but it is not impossible to do it with 80 line length and small display either. There is room for improvement here, e.g. the tool could detect such spots in our code and suggest changes, so that formatting would be better with the default settings. However, that is outside of the scope of pana and pub.dev, and should be discussed e.g. on dart_style.

Closing this, as pub-dev has no plan to implement a different formatting analysis other than relying on the defaults of dart format.

@isoos isoos closed this as completed Jan 12, 2022
@blagoev
Copy link

blagoev commented Jan 12, 2022

@isos dart format does accept different line lengths with -l option
-l, --line-length Wrap lines longer than this.

dart format -h
Idiomatically format Dart source code.

Usage: dart format [options...] <files or directories...>
-h, --help                     Print this usage information.
-v, --verbose                  Show all options and flags with --help.
-o, --output                   Set where to write formatted output.

          [json]               Print code and selection as JSON.
          [none]               Discard output.
          [show]               Print code to terminal.
          [write] (default)    Overwrite formatted files on disk.

    --set-exit-if-changed      Return exit code 1 if there are any formatting changes.
    --fix                      Apply all style fixes.
-l, --line-length              Wrap lines longer than this.
                               (defaults to "80")

Run "dart help" to see global options.

@esDotDev
Copy link

There is room for improvement here, e.g. the tool could detect such spots in our code and suggest changes, so that formatting would be better with the default settings.

I think the real room for improvement requires small changes from both teams.

I don't see any reason to close this as the original issue still stands. Just because there is no immediate fix, doesn't mean this issue ceases to be a problem.

@isoos
Copy link
Collaborator

isoos commented Jan 12, 2022

dart format does accept different line lengths with -l option
[...]
pub.dev should respect that line-length, and use it when determining whether a library is dart formatted or not

pub.dev / pana will run dart format to check if the tool finds the file being formatted. If/when they are formatted, we will accept them as-is. Whether the tool decides this via an inline annotation in the .dart file, or a package-level setting in the pubspec.yaml is outside of the scope of the package analysis, we will follow whatever the established default will be.

Also note: not all packages get maximum pub points on pub.dev, yet, they will be ranked in search just fine, as we also include popularity and likes when calculate the default ranking. It is okay to not get all the possible points.

@lukepighetti
Copy link

lukepighetti commented Jan 13, 2022

80 characters is just too short for any reasonable Flutter code, for what it's worth.

All of the Flutter code inside Google fits within that without undue pain.

It looks like flutter/flutter is not using dartfmt or the strict 80 character line length. Maybe that's considered "outside" Google? https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#formatting

@munificent
Copy link
Member

It looks like flutter/flutter is not using dartfmt or the strict 80 character line length. Maybe that's considered "outside" Google? https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#formatting

That's correct. Flutter does not use dartfmt or follow "Effective Dart", both of which are required in order to commit Dart code inside Google.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests