[WIP] Code hygiene

[pablosichert]

This PR introduces a plugin that transforms all code automatically to conform with linting rules.

With this change we can run

npm run lint

on the CI build and fail it if there's some inconsistency caught by the linter.

There's about 1500 errors left that need to be resolved manually.

EDIT: If we leave out prop-type checking, there's only about 100 errors left.

(edit by ts: #1206)

[metas-ts]

Hi @pablosichert ,
we are happy that you proactively go about improving the codebase, but think that this PR is too much at once..

• there are a some ongoing issue branches and we expect there will be merge conflicts, so someone would need to coordinate with @ottosichert and @wiadev to make sure those conflicts are dealt with (e.g. by also running those tool(s) in the respective branches)
• Both codeclimate and codacy see a lot of new issues..now I don't know javascript myself and I trust humans more than static code review tools, but still this makes me very nervous..
  At least in java, so far when I saw both the issue-count raise and the GPA drop at the same time, and then looked at the changes, it never looked well..
• ..and here, because of the mass of changes, I can't even see what the tools actually complain about.
• We can't see any "real" code changes because of the formatting change. There might be stuff that worked the way it was, even if the code was bad (just ask the openSSL guys :-) )
• I'm also all in favour of a standardized formatting scheme, but then
  • it would imho make sense to coordinate with and get agreement from the other devs that this formatting schema shall be used from now on and
  • make one PR that contains only the formatting change. Then we could integrate that without looking that the changes, and still be sure that no logic changed.

I'm not sure if I made it clear enough: I appreciate your effort a lot. Still, I'm afraid it might cause more trouble than good.

WDYT?

[pablosichert]

Hi @metas-ts, thanks for having a look at this - your concerns are more than valid.

• If this tool is applied on Implements host application plugins support #1229, the merge shouldn't have a big complication, but I can definitely verify that and will help with the transition.
• I removed the prop-type rule and the issue count dropped by 1300. I will have a look at what went wrong elsewhere, but this PR should definitively be able to reduce the total amount of issues significantly.
• Good point about the functionality. I could cherry-pick the changes that remove unused variables etc., but in that case code climate will most likely complain. I can assure there was no complex logic changed and almost all of it were unused variables that do nothing but contribute to code smell.
• What's really hurting this refactoring is that there is no test suite at all to validate the changes against. After this PR my plan is to introduce a test setup, so at least all code going forward will be tested and we can guard ourselves against regressions in the future.

I really think it would be beneficial in the long run to apply this PR now, even though we can't be sure that nothing breaks. We might run into a couple of bugs along the way, but those we'll finally be able to tackle in a automated regression test.

I can't estimate the potential short-term business impact on this one, and would like to defer that question to those holding a stake in this regard. But I advise that going forward with such an unhealthy code base will lead to choking in the long run.

I have no intention to merge this until we didn't actually get the total number of issues down that are reported by the external tools.

@ottosichert, @wiadev - please join in on this discussion.

[ottosichert]

Summary

• I second this change because it must be done eventually

• Create new config only PR and settle on eslint config with all devs

• @metas-ts and @metasnw have to decide on this effort

---

Hello,

• there are a some ongoing issue branches and we expect there will be merge conflicts, so someone would need to coordinate with @ottosichert and @wiadev to make sure those conflicts are dealt with (e.g. by also running those tool(s) in the respective branches)

This morning @pablosichert informed me about this PR he had planned - there are no branches open I work on which would conflict with this PR (I will continue with #1239 after this discussion).

I don't know about @wiadev but I see there still is this PR open: #1229. But also:

• If this tool is applied on Implements host application plugins support #1229, the merge shouldn't have a big complication, but I can definitely verify that and will help with the transition.

---

• Both codeclimate and codacy see a lot of new issues..now I don't know javascript myself and I trust humans more than static code review tools, but still this makes me very nervous..
  At least in java, so far when I saw both the issue-count raise and the GPA drop at the same time, and then looked at the changes, it never looked well..

l looked through the changes made by prettier in b5d6723 and I couldn't spot any severe issues. I've personally never used prettier or a similar tool as I always start with a strict eslint config such as airbnb, react/recommended and eslint/recommended.

• ..and here, because of the mass of changes, I can't even see what the tools actually complain about.

I can confirm codebeat collapses under the amount of issues and doesn't show anything. My experience with codebeat showed that some rules are too strict when used with react, e.g. BLOCK_NESTING of [3, ...]. So this could contribute to the high count of new issues introduced. E.g.

if (foo) {
  list.forEach((item) => {
    // good?
    item.bar && item.baz();

    // bad
    if (item.bar) {
      item.baz();
    }
  }
}

• We can't see any "real" code changes because of the formatting change. There might be stuff that worked the way it was, even if the code was bad (just ask the openSSL guys :-) )

AFAIK prettier only applies fixes for formatting rules and no real codemods like no-extra-bind (see https://github.com/prettier/prettier#how-does-it-compare-to-eslint-or-tslint-stylelint).

• I'm also all in favour of a standardized formatting scheme, but then
  • it would imho make sense to coordinate with and get agreement from the other devs that this formatting schema shall be used from now on and
  • make one PR that contains only the formatting change. Then we could integrate that without looking that the changes, and still be sure that no logic changed.

I totally agree here. I sent @pablosichert my preferred .eslintrc but I don't know what the rest of the team prefers. Splitting the PR into configuration and actual formatting change is the cleanest way to go.

Also a dedicated PR for the eslint rules allows for fine-grained coordination of all team members by review comments per line.

The only concern I really have is the missing testing suite.

• What's really hurting this refactoring is that there is no test suite at all to validate the changes against. After this PR my plan is to introduce a test setup, so at least all code going forward will be tested and we can guard ourselves against regressions in the future.

Setting it up is essential and will benefit this codebase more than any new feature.

This is why I also think it is a good idea to resolve this issue now. I am aware this workload will prevent us from delivering new features and important bugfixes for at least two days (excluding setting up the testing suite) but I strongly suggest this move as it's only a question of time when this step has to be done.

The point is, you, @metas-ts and maybe @metasnw, have to approve this step as you can assess the economic practicability.

[pablosichert]

Just to be clear on this one:

I don't think we need to rush a decision immediately - I can discard the changes and apply them at a later point, since most of the changes are done automatically anyway.

But I propose that we pin down a specific date, as soon as possible, where we apply those drastic changes spanning over the whole code base. In any case, there won't be a perfect point in time to do it.

[metas-ts]

Hi @pablosichert @ottosichert
Thx for your feedback & clarifications.
I would suggest that you guys coordinate with @wiadev to make sure this isn't a a problem for him, and if noone (including @metas-mk @teosarca @metasnw ) has objections, we could integrate the PR into master after having prepared the next RC (i.e. next friday).
That way we have two weeks until the change become part of the weekly release.
WDYT?

Some further notes from my side:

• for the java codebase we are sooner or later going to pick one static review tool and and customize its rules to our needs. Feel free to do the same within the js codebase
• about the test suite: we also find it important, so whatever you might need, please ask and we will see how we can pave the way. Are you aware of the metasfresh-webapi-dev and metasfresh-db docker images? Depending on what sort of testing you have in mind, they might be helpfull.

[teosarca]

agree

[pablosichert]

Sounds good 👍🏻 Let‘s aim to not have any open PRs on Friday evening, so we can make the transition as easy as possible.

[teosarca]

until friday, let's see how many conflicts we will accumulate here.

If it's easy to do it, i would suggest to redo this instead of handling conflicts if they are to many (i find it error prone).

[pablosichert]

Yes, there is no point in picking this up when master has moved on. Closing this and will reevaluate again on Friday evening.