Skip to content

Packages with formatting issues should not have reduced score #2322

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

Closed
cedx opened this issue May 23, 2019 · 9 comments
Closed

Packages with formatting issues should not have reduced score #2322

cedx opened this issue May 23, 2019 · 9 comments

Comments

@cedx
Copy link

cedx commented May 23, 2019

Code formatting is a personal preference. This does not necessarily indicate that a package is of poor quality. Packages following the style conventions can be of very poor quality (security flaws for example).

However, the score of packages that do not use the style conventions defined by the Dart team are now penalized on the Pub website.

I find it really weird: I'm not going to stop using the Dart SDK because it's not coded as I would have liked. Yet the Dart SDK does not apply the stylistic conventions you have defined, far from it.

It's a bit like saying: "do what I say, not what I do". It's unfair.

@isoos
Copy link
Collaborator

isoos commented May 24, 2019

I think code readability is an important aspect of a library. When you import a third-party lib, you trust its developer, but also want to keep your options open: if you need to, you would like to dig into the source code, interpret it, and if needed, fix it.

Code style (whether it is dartfmt formatting or the use of braces, capitalization or use of other language elements) is an important part of the code readability (along with how you structure and organize the code). It is by definition opinionated, and follows one standard or another.

You are free to use your own preferred style, you can publish your package, and people can use it - there is no blocking in it. However, pub site will check against a common set of standards that the Dart team thought to be good conventions, and will expose it as part of the package score. Pub site aims to provide a consistent quality score that factors in everything, helping users to decide to choose using a package. By checking against this common set of standards users of a package can at least have some expectation against code quality, before even looking at the source code itself.

If you think that some convention is not right, please raise an issue against dartanalyzer, dartfmt or package:pedantic, as we will follow them as they evolve this ruleset. If you think that the penalty for a category of issue (whether it is formatting or a dartanalyzer warning) is too excessive, open an issue for pana.

@isoos isoos closed this as completed May 24, 2019
@jonasfj
Copy link
Member

jonasfj commented May 24, 2019

Afaik the set of lints in pedantic is pretty limited..

It's also possible that in some future we could support custom linter rules..

But honestly, I hope the community creates a reasonable set of linter rules that everybody can use. Look at golang, everything is consistently formatted. This has a lot of value, personally, I'm not a fan of everything dartfmt does, but I think that following it far out weights the pain.

@jonasfj
Copy link
Member

jonasfj commented May 24, 2019

Just to be clear: For now I think it's best if we stick to what we have, and keep it simple.

@cedx
Copy link
Author

cedx commented May 24, 2019

My use case: I don't follow all Dart style conventions because I don't like some. But I always apply dartfmt before publishing a package to ensure that others can read my code easily.

The issue: dartfmt does not fix some lints like DO use curly braces for all flow control structures.
So most of my packages are now penalized because some if statements don't have curly braces.

Personally (and at my work, 2/3 of the developers also think that), if an if statement is very short (less than 40 characters), I find it easier to read:

if (condition) doSomething();

... than:

if (condition) {
  doSomething();
}

// or worst: if (condition) { doSomething(); }

I don't intend to change my code just for stylistic reasons. And @munificent (the lead maintainer of dartfmt) does not intend to fix it either.

dart-lang/dart_style#578
dart-lang/dart_style#664
(and some other issues)

@jonasfj
Copy link
Member

jonasfj commented May 24, 2019

Hopefully it's a small penalty..

I haven't tried it, but maybe you could start the file with // ignore: curly_braces_in_flow_control_structures (probably right before the library ... statement, if I were to guess).


Maybe we should respect whatever analysis_options.yaml file a package contains. There is so many things to improve on our package analysis, that's one of the downside with doing package analysis -- we'll never make everybody happy.

@cedx
Copy link
Author

cedx commented May 24, 2019

Especially, code formatting: this is an endless subject! 😄

@jonasfj
Copy link
Member

jonasfj commented May 24, 2019

Code formatting is, yet somehow in golang they managed to all agree on something.

Persinally, I won't say that I loved everything gofmt did when I was working in golang, but I decided to "Drinking the Kool-Aid" 🙈😁

Because to me it's better to follow less than ideal lints/formatting rules, if it means everybody does the same thing. But that's just my personal opinion :)

(After a while coding in golang I was more concerned with deadlocks and race conditions than code formatting anyways, hehe 😁)

@isoos
Copy link
Collaborator

isoos commented May 24, 2019

@cedx: I'm pretty sure that one-liner if statements are allowed without curly braces.

Also worth to point out that dartfmt never modifies the non-whitespace characters, only indents the code, and I believe this is how it should be doing it. Adding curly braces should be your decision.

As Jonas pointed out, you can tag your source files to ignore specific lints you don't agree with, however, in the future we may also expose that in the analysis.

In the past we had many request to tune the analysis options in one way or the other, to enable more things or to make it more strict. I believe the current approach to follow the recommended standards and defaults is a good balance, and we will keep it over the developer-provided one.

@natebosch
Copy link
Member

I'm pretty sure that one-liner if statements are allowed without curly braces.

They are.

dartfmt never modifies the non-whitespace characters, only indents the code

It does have a few --fix flags that let users opt in to some non-whitespace changes. I reopened dart-lang/dart_style#664 so that we could discuss whether these braces should be one of those flags.

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

4 participants