Bit: Linting policy and issues

[ranm8]

Friends,
I wanted to follow up with an issue to improve linting experience on Bit and make sure all of us are on the same page.
Please contribute best practices we can adopt.

Few things I would really like.

Constructors

constructors are auto-formatted after commit through prettier to a one liner which makes the code far less readable:

constructor(private workspace: Workspace, private scripts: Flows) {}

I would really like it to include one dependency at a line like this:

  constructor(
    private workspace: Workspace, 
    private scripts: Flows

Return types

I would really like to force return types on all signatures.

Cyclomatic complexity

Cyclomatic complexity checks the how complex a single unit of code is and scores it according to the possible amount of paths. I think it could highly improve us. Anyone knows a good linter for that?

Goal is to eventually make sure we are following up on that and highly reduce the amount of rules we disable. We will have a better codebase and everyone will be happier.
@GiladShoham @qballer @davidfirst @imsnif

[GiladShoham]

I disagree with the fact that multiline constructor is more readable. (there is a wide debate about it. the rationale behind it is to be similar to class parameter properties )
Anyway, I prettier doesn't support it yet (but you are not the first to ask it) see - https://github.com/prettier/prettier/issues/5666
And I don't see it as a strong enough reason to disable prettier.

Regarding return types, It's a great rule, the issue is that it will enforce you to fix hundreds of legacy functions...

re Cyclomatic complexity, I don't know on top of my head on something like this.

I have some other thoughts about linting I'll post later.

[davidfirst]

About the constructor, personally, I don't mind either way. (we do want the prettier though for sure).

The return type - I'd love to see as must as possible return types. Gilad's point is correct, it won't be easy to enforce it by a lint due to the current code.

Cyclomatic complexity - it's already built-in in ESLint, we just need to enable the rule: "complexity": ["error", { "max": 2 }],. (fun fact: turn this on and you'll get 964 errors. The most problematic method is processOneDepFile with the complexity of 30.). We can add this rule with a high complexity threshold in order to avoid fixing the current code.

About lint rules.
I'm good with the current rules we have. @GiladShoham , please correct me if I'm wrong, but I think you're good with them as well.
I wouldn't suggest adding more rules as it'll probably be a time-consuming task to fix the current code.
The question is about the current rules whether some should be disabled.
@ranm8 , @qballer , @imsnif , I don't want that this task will take longer than it should (it has the potential to be so). So, only if you strongly disagree with some rules, please mention them here with a good reason why you don't like them.

Once all of us (or at least most of us) agree on the rules, then, there is no justification for disabling the rules on the file level.

[GiladShoham]

I want to make no unused vars and no console errors instead of warnings. Since it's warnings and not errors people tend to ignore because the ci is green.
The no console is very important. A console in the code might break json outputs or layout etc.
The console logs should be in very specific files ignored explicitly.

[qballer]

constructor - I'm for break line on each dependency.
default export there is a rule compelling you to do default export. it's annoying.
unused vars someArray.map( (_, i) => i*2) should be legal even though _ isn't being used.

[imsnif]

Just chiming in to say that I don't care about lint rules as long as we have them. I'm good with whatever you decide.

[ranm8]

Friends, I guess there is no effective linter for this but please, keep all of your 100% of your extension public API fully documented including full type signature including return type.
No API can be added without proper documantation.

  /**
   * list all workspace components.
   */
  async list(): Promise<Component[]> {
    const consumerComponents = await this.componentList.getAuthoredAndImportedFromFS();
    return this.transformLegacyComponents(consumerComponents);
  }

[davidfirst]

a quick summarize about the lint rules:

rules to add/change

• complexity with a high threshold.
• no-console change to be an error.

The only disagreement we have is about the no-unused-vars rule.
@GiladShoham wants it to be an error, @qballer doesn't want it.
The status is much better than I thought :) only one disagreement.

@qballer , please provide a good reason why you don't want it.
I'm coping here the reason for this rule from the eslint website

Variables that are declared and not used anywhere in the code are most likely an error due to incomplete refactoring. Such variables take up space in the code and can lead to confusion by readers.

To me, the most important is the last part. It makes the code less readable and confusing.

[GiladShoham]

Few more notes
Please stop using bitwise operation. It's not readable and there is no reason to use it. There is already a rule for this. Stop ignore it.
Re constructor break line, it's not possible.
Re default export, I disabled this rule 2 months ago.
Re un used vars, I see your point @qballer. Let's see if we can identify unused vars vs unused function args.

[qballer]

function arguments unused should be fine if they prefix with an _ (underscore). That is the rule I want.

I think warning is silly. it should error or not be there as a rule of thumb.

[davidfirst]

Please stop using bitwise operation. It's not readable and there is no reason to use it

100% agree!

I think warning is silly. it should error or not be there as a rule of thumb.

Totally agree. The CI anyway doesn't enforce warnings, so they just accumulated and no one fixes them.

function arguments unused should be fine if they prefix with an _ (underscore). That is the rule I want.

@qballer , the underscore variable _, is already exempt from this rule. Do you have a use-case of more than one variable? If so, a regex can be used for any variable that starts with _. To me, it's still confusing though.

[qballer]

this is fine too @davidfirst
[].map((_item, _index, arr) => arr )

[davidfirst]

@qballer , I see your point. So, there are cases when more than one unused argument is needed.

I just realized that eslint has already an option to allow these cases: no-unused-vars: ["error", { "args": "after-used" }].
The idea is basically to allow unused arguments when you do use the last argument. You can see the link above for concrete examples.

If so, @qballer , enabling this option should cover your cases. Correct?

[qballer]

Most of the time what you provided should be enough. I just don't get it, why not let typescript compiler do it.
--noUnusedParameters
--noUnusedLocals
https://www.typescriptlang.org/docs/handbook/compiler-options.html

[davidfirst]

@qballer , ok, great. So we have a conclusion regarding the no-unused-vars then.

why not let typescript compiler do it.

We have a linter for lint, typescript for types. I would keep their responsibilities separated unless you see a good reason why not doing so.

[ranm8]

Guys, If we are covered than David please go ahead and apply the rules. I agree re warnings, let’s just use errors or leave aside.

[davidfirst]

Completed the eslint changes. See the PR https://github.com/teambit/bit/pull/2549 for more details.