Disable no-underscore-dangle rule

[rawagner]

The rule does not allow underscore in plugin components but is widely used in console. I'd like to disable it to allow plugin components use the same convention as rest of console's code.

See #1803 (comment)

@vojtechszocs @spadgett @christianvogt any objections ?

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rawagner
To complete the pull request process, please assign kyoto
You can assign the PR to them by writing /assign @kyoto in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@rawagner: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-console-olm	4a60e8b	link	/test e2e-aws-console-olm
ci/prow/e2e-aws	4a60e8b	link	/test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[christianvogt]

I'm in favor of this rule to enforce use of typescript private vs underscore naming convention. I'm not sure if there's a way to allow ending a name with underscore while preventing the prefix.

I think a better convention would be to name classes according to their hoc enhancement rather than adding an underscore suffix to the base component.

[spadgett]

We might be able to use enforceInMethodNames

https://eslint.org/docs/rules/no-underscore-dangle#enforceinmethodnames

I think a better convention would be to name classes according to their hoc enhancement rather than adding an underscore suffix to the base component.

Agree. It would be good to come up with a consistent naming convention for connected components.

[christianvogt]

If we adopt the convention of exposing a single default component per file, then this problem goes away as the hoc enhanced component is default exported inline and didn't require a name.

Eg. https://github.com/openshift/console/blob/master/frontend/packages/dev-console/src/components/EmptyState.tsx

[vojtechszocs]

👍 on using TypeScript private modifier in favor of underscore naming convention.

However, this doesn't apply to React (function or class) component declarations, where a trailing underscore was used to denote that the given component is internal / inner / part of another component.

I'm not sure if there's a way to allow ending a name with underscore while preventing the prefix.

Sadly, the no-underscore-dangle rule doesn't seem to support configuration that only allows single trailing underscore for function or class declarations.

I think a better convention would be to name classes according to their hoc enhancement rather than adding an underscore suffix to the base component.

This reminds me of some projects using e.g. ConnectedFoo for Foo component. But as @rawagner said, we already have the trailing underscore applied consistently within the public directory.

We might be able to use enforceInMethodNames

Yes, but that only covers object property names, not declared variable names themselves.

If we adopt the convention of exposing a single default component per file, then this problem goes away as the hoc enhanced component is default exported inline and didn't require a name.

I agree with that. Code under packages should follow single default component export per file convention.

If some package isn't ready to follow that convention, it can add e.g. packages/foo-pkg/.eslintrc file that extends packages/.eslintrc.js and suppresses no-underscore-dangle until its code is updated.

If we decide to follow some convention, we should apply it consistently to all code under packages, keeping in mind that public code will be moved to packages eventually.

[vojtechszocs]

TL;DR version

If some package isn't ready to follow that convention, it can add e.g. packages/foo-pkg/.eslintrc file that extends packages/.eslintrc.js and suppresses no-underscore-dangle until its code is updated.

[christianvogt]

@vojtechszocs well put. Override for the particular rules only within the particular package. This should be seen as a temporary fix and teams should then work towards adopting the primary rule set.

[rawagner]

Ok, thank you all for discussion :)

Lets keep the rule enabled and follow convention of exposing a single default component per file. If some plugin isn't ready to do that they could disable the rule in their .eslintrc.js

Closing.