Remove custom curly rule from canvas

[stacey-gammon]

Remove the eslint rule for canvas that will cause this code:

if (true) {
 console.log('hi');
}

to be reported as an error, and auto fixed to this code:

if (true) console.log('hi');

Our style guide explicitly states the former is better . So I thought about adding a rule to be explicit about our style guide suggestion which, imo, being explicit is better than not. But then we also talked about just adhering to the airbnb style guide, which says you can do either. So rather that really get into the weeds about the right rules we should be using, which would end up being a much bigger conversation, I think the best short term strategy is just to remove the error output.

Oh and actually I think I'm noticing it so much because I believe the tslinter actually prefers the first and will auto correct to it.

[w33ble]

@stacey-gammon don't forget to cd x-pack/plugins/canvas && node scripts/lint.js --fix (you can do it from the root, but it's wildly slower).

The output looks mostly correct, but I'm reminded of why I set that rule in the first place. The else part of conditionals isn't correctly bracketed:

diff --git a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/ply.js b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/ply.js
index f3f1672759..b0c995e048 100644
--- a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/ply.js
+++ b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/ply.js
@@ -103,9 +103,9 @@ export const ply = () => ({
     const datatablePromises = originalDatatables.map(originalDatatable => {
       let expressionResultPromises = [];
 
-      if (args.expression)
+      if (args.expression) {
         expressionResultPromises = args.expression.map(expression => expression(originalDatatable));
-      else expressionResultPromises.push(Promise.resolve(originalDatatable));
+      } else expressionResultPromises.push(Promise.resolve(originalDatatable));
 
       return Promise.all(expressionResultPromises).then(combineAcross);
     });

There's a handful of these cases, where the rule I chose produced consistent output, which is why I went with it.

You're welcome to clean those things up by hand, of course, but it means that the linter isn't going to catch those uses, even though they conflict with the styleguide.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[stacey-gammon]

So weird, I ran node ../../../scripts/eslint.js --fix --debug ./public/functions/asset.js in the canvas dir and no changes came up so I thought the lack of a rule meant, do whatever you want, it won't auto fix. Any idea of why the difference, @spalger?

It looks also like there is existing code outside of Canvas that mixes the rule. And if I write in non canvas code, there is no lint error reported:

That's why I thought this change would be simple, if we didn't auto fix anything, just stopped the auto fixing (until we can figure out a more consistent path forward - e.g. actually setting a rule and then fixing everything).

so, thoughts on removing this without auto fixing anything?

[w33ble]

thoughts on removing this without auto fixing anything?

The CI won't pass that way, as you can see in the build failure above (more specifically, in this failure).

[clintandrewhall]

This applies towards #25753... I appreciate you working on this while I tackle i18n. You're awesome, @stacey-gammon...!

[stacey-gammon]

ahhhh, okay, it's because if it's on a single line, it's acceptable to the linter (e.g. if (true) return true; passes. That's why the auto fix doesn't fix the second line in your example - it fits on one line, and also why the OSS code I linked too passes. So it may look weird, but there is an explainable reason for why it didn't add braces for the second else.

[stacey-gammon]

Also, I don't know why it didn't work earlier but this time around i ran

node scripts/eslint --no-cache "./x-pack/plugins/canvas/**/*.js" --fix

in kibana dir and it worked. Maybe --no-cache, or maybe I just wasn't patient enough earlier (it was taking forever to finish... maybe some other issue)? 🤷‍♀️

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[w33ble]

it's because if it's on a single line, it's acceptable to the linter

So, the tooling doesn't actually match the styleguide? It's a pretty hard and clear rule that one-line conditionals are never acceptable:

---

Always use braces for conditionals and loops

// good
if (err) {
  return cb(err);
}

// bad
if (err) cb(err);

// bad
if (err)
  return cb(err);

---

Even with this change, our tooling produces code that is in violation of the styleguide. This needs to be fixed upstream I think, right?

[w33ble]

This violates the styleguide: https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#always-use-braces-for-conditionals-and-loops

I think the requirement of human review for this seems to indicate a problem upstream though.

[w33ble]

Just for clarity, I'm totally cool with you merging this ahead of an upstream fix as long as you manually fix these.

[w33ble]

This violates the styleguide: https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#always-use-braces-for-conditionals-and-loops

[clintandrewhall]

+1 This change should not require human review and produce code that matches the style guide. If anything, this PR is a great test case for the effectiveness of the rule.

[stacey-gammon]

I touched on this in the initial comment - our style guide and the airbnb style guide conflict. The linter rule is inline with the airbnb style guide. If we decide there is no good reason to deviate from the airbnb style guide, then those specific one liners are allowed to stay and would not require any manual intervention.

The situation prior to this PR was:

1. Our linting rules are too lenient and do not catch all mistakes according the Kibana style guide section (but do match the airbnb style guide section). You can get away with writing if (err) cb(err); without a lint error.
2. The canvas linting rules were explicitly different than the Kibana linting rules, and auto-corrected to the bad version of the rule. You could not write code that matched the styleguide without a lint error.

The situation with this PR in its current form:

1. [same] Our linting rules are too lenient and do not catch all mistakes according the Kibana style guide section (but do match the airbnb style guide section). You can get away with writing if (err) cb(err); without a lint error.
2. Canvas follows suit and is no different than the rest of Kibana.

I agree there is a more ideal situation:

1. We determine which curly rule we want to have - airbnb or the Kibana one.
2. We update our styleguide - if airbnb, we remove that snippet all together. If there is a legitimate reason to deviate from airbnb, then lets leave it in there.
3. If neccessary, we update the linting rule to match the decision made.
4. If neccessary, we auto fix all of Kibana code so the linting passes.

This PR tackles the short term approach. I don't think I should need to manually tweak all of Canvas's code in order to get this merged (which would still leave Kibana code containing the accepted one liners).

I can create an issue for the longer term approach, since I agree the situation as it stands is not ideal, as our rule does not accurately reflect the style guide.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[w33ble]

I don't think I should need to manually tweak all of Canvas's code in order to get this merged

You're trying to enforce the styleguide, and you've introduced 2 changes that do not follow it. It seems reasonable to ask you to fix them in review.

[w33ble]

If we decide there is no good reason to deviate from the airbnb style guide, then those specific one liners are allowed to stay and would not require any manual intervention.

I thought we already went through that, but I can't find the issue where this would have been discussed. The history of the styleguide is interesting though, we used to only require curlies for multi-line, and prefer them over single-line conditionals. Then later, in #7435 the rule became always use curlies, no exception. So maybe the styleguide was changed but the linter was never actually updated to enforce it correctly?

[stacey-gammon]

You're trying to enforce the styleguide, and you've introduced 2 changes that do not follow it. It seems reasonable to ask you to fix them in review.

That code existed prior to this PR, I didn't introduce it. I used automatic fixing to make sure canvas code adheres to Kibana's current linting rules. The second part - changing the linting rules to reflect the style guide, is a separate issue that can be tackled in another PR, by anyone who has a desire to do so. And at that point, automatic fixing can be used to bulk fix the places in canvas, and elsewhere, in one go.

I re-opened #11398 and asked Kibana folks to weigh in so we can figure out which direction to go in for the longer term solution.

[spalger]

I think we can close this PR with #27176