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

[Styleguide] Revert chain indentation rule or update the entire codebase #12729

Closed
Bargs opened this issue Jul 10, 2017 · 6 comments
Closed

[Styleguide] Revert chain indentation rule or update the entire codebase #12729

Bargs opened this issue Jul 10, 2017 · 6 comments
Labels
chore discuss stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@Bargs
Copy link
Contributor

Bargs commented Jul 10, 2017

The chain indentation rule in our styleguide was inverted awhile back without much thought by #7435. A styleguide should create consistency, this rule is having the opposite effect. We should either revert the rule change or fix the entire codebase in one go (I'd be in favor of simply reverting). Applying this rule selectively to new code only makes the codebase worse.

@epixa
Copy link
Contributor

epixa commented Jul 10, 2017

I proposed the change precisely because the existing style rule was inconsistently being applied across the codebase despite being documented. I'm not sure why that was the case, though I have ideas. We should bulk update the codebase on styleguide change wherever practical though.

@kimjoar
Copy link
Contributor

kimjoar commented Jul 10, 2017

If it's in the styleguide I think eslint should fail on it. Then we're pushed towards either removing it or doing a bulk update. Imo PRs shouldn't stall because of style discussions, unless it's a "new problem" we haven't covered yet.

@kimjoar
Copy link
Contributor

kimjoar commented Jul 10, 2017

(unless it's things eslint can't easily check, of course)

@Bargs
Copy link
Contributor Author

Bargs commented Jul 10, 2017

@kjbekkelund totally agree. I'd take it a step further and say new styleguide rules shouldn't be introduced without an accompanying eslint rule (if possible) and a bulk update to existing code. Otherwise the new rule only creates more inconsistency, as is happening here.

@kimjoar
Copy link
Contributor

kimjoar commented Jul 10, 2017

We could even move the explanation of style rules into the eslint config itself, "coupled" to the eslint rule that enforces it. Then the Kibana styleguide becomes just the higher-level "non-eslint-able" stuff. And it should be way easier to keep eslint and explanation consistent too.

(Or maybe we should just explore Prettier, and not have to think about (and discuss) these things at all 🙈)

@timroes timroes added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Aug 17, 2018
@timroes
Copy link
Contributor

timroes commented Aug 17, 2018

I currently assigned the platform team to this, due to owning eslint and prettier (afaik). Please feel free to close this issue, if it's no longer relevant.

@joshdover joshdover added the stale Used to mark issues that were closed for being stale label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore discuss stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

5 participants