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

💅 lint/complexity/noForEach should not throw on Effect.forEach (or be configurable) #3351

Open
1 task done
nickrttn opened this issue Jul 4, 2024 · 8 comments · May be fixed by #4498
Open
1 task done

💅 lint/complexity/noForEach should not throw on Effect.forEach (or be configurable) #3351

nickrttn opened this issue Jul 4, 2024 · 8 comments · May be fixed by #4498
Labels
A-Linter Area: linter good first issue Good for newcomers L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@nickrttn
Copy link

nickrttn commented Jul 4, 2024

Environment information

CLI:
  Version:                      1.8.3
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-kitty"
  JS_RUNTIME_VERSION:           "v20.12.2"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/9.4.0"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 false

Linter:
  JavaScript enabled:           true
  JSON enabled:                 true
  CSS enabled:                  true
  Recommended:                  true
  All:                          false
  Enabled rules:
  performance/noDelete
  suspicious/noCatchAssign
  suspicious/noUnsafeNegation
  complexity/useLiteralKeys
  style/useImportType
  complexity/noMultipleSpacesInRegularExpressionLiterals
  a11y/useValidLang
  complexity/noUselessEmptyExport
  suspicious/useNamespaceKeyword
  suspicious/useValidTypeof
  a11y/useValidAriaRole
  correctness/noConstantCondition
  a11y/useAriaActivedescendantWithTabindex
  suspicious/noAssignInExpressions
  style/useDefaultParameterLast
  complexity/noEmptyTypeParameters
  correctness/noConstructorReturn
  style/useSelfClosingElements
  suspicious/noDuplicateParameters
  style/useTemplate
  correctness/noUnusedLabels
  complexity/noUselessTernary
  correctness/noUnreachableSuper
  suspicious/noCompareNegZero
  suspicious/noExplicitAny
  correctness/noSwitchDeclarations
  a11y/noAutofocus
  correctness/noUnsafeOptionalChaining
  correctness/noConstAssign
  suspicious/noControlCharactersInRegex
  complexity/noUselessTypeConstraint
  style/noVar
  suspicious/noDoubleEquals
  suspicious/noRedundantUseStrict
  style/useLiteralEnumMembers
  nursery/noUnknownPseudoClassSelector
  suspicious/noEmptyInterface
  nursery/noInvalidPositionAtImportRule
  suspicious/noConstEnum
  nursery/noDuplicateElseIf
  correctness/noPrecisionLoss
  nursery/noUnmatchableAnbSelector
  suspicious/noGlobalIsNan
  nursery/noUnknownSelectorPseudoElement
  correctness/noStringCaseMismatch
  nursery/noUnknownFunction
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noImplicitAnyLet
  suspicious/noFallthroughSwitchClause
  a11y/useKeyWithClickEvents
  correctness/noUnreachable
  nursery/noImportantInKeyframe
  suspicious/noDuplicateObjectKeys
  complexity/noUselessThisAlias
  complexity/noThisInStatic
  complexity/useOptionalChain
  correctness/noInnerDeclarations
  style/noParameterAssign
  suspicious/noDuplicateCase
  suspicious/noUnsafeDeclarationMerging
  nursery/noLabelWithoutControl
  a11y/useValidAnchor
  suspicious/noMisleadingCharacterClass
  complexity/useRegexLiterals
  correctness/noSelfAssign
  suspicious/noRedeclare
  style/noUselessElse
  style/useShorthandFunctionType
  suspicious/noShadowRestrictedNames
  a11y/useMediaCaption
  complexity/noUselessLabel
  complexity/noUselessCatch
  nursery/noDuplicateFontNames
  correctness/noUnsafeFinally
  a11y/useAriaPropsForRole
  correctness/noNonoctalDecimalEscape
  style/useEnumInitializers
  a11y/useHtmlLang
  suspicious/noDuplicateTestHooks
  complexity/noStaticOnlyClass
  nursery/noShorthandPropertyOverrides
  style/useWhile
  complexity/useArrowFunction
  style/noInferrableTypes
  a11y/noNoninteractiveTabindex
  complexity/useSimpleNumberKeys
  correctness/useYield
  a11y/noInteractiveElementToNoninteractiveRole
  nursery/noEmptyBlock
  correctness/noUnnecessaryContinue
  style/useNumericLiterals
  suspicious/noApproximativeNumericConstant
  suspicious/noImportAssign
  correctness/noGlobalObjectCalls
  suspicious/noLabelVar
  suspicious/useDefaultSwitchClauseLast
  a11y/useAltText
  correctness/noEmptyCharacterClassInRegex
  suspicious/noSuspiciousSemicolonInJsx
  suspicious/noSparseArray
  a11y/useIframeTitle
  complexity/noBannedTypes
  a11y/noSvgWithoutTitle
  correctness/noVoidElementsWithChildren
  style/useAsConstAssertion
  correctness/useJsxKeyInIterable
  style/useExportType
  complexity/noUselessLoneBlockStatements
  style/noArguments
  a11y/useValidAriaValues
  suspicious/noDebugger
  suspicious/noCommentText
  suspicious/noGlobalAssign
  suspicious/noMisleadingInstantiator
  suspicious/noPrototypeBuiltins
  suspicious/noDuplicateJsxProps
  suspicious/noThenProperty
  a11y/noPositiveTabindex
  correctness/noEmptyPattern
  complexity/noExcessiveNestedTestSuites
  nursery/noDuplicateJsonKeys
  a11y/useKeyWithMouseEvents
  security/noDangerouslySetInnerHtmlWithChildren
  suspicious/noExtraNonNullAssertion
  correctness/noRenderReturnValue
  correctness/useExhaustiveDependencies
  security/noGlobalEval
  style/noNonNullAssertion
  a11y/noRedundantRoles
  complexity/useFlatMap
  correctness/useIsNan
  style/useConst
  suspicious/noGlobalIsFinite
  suspicious/noSelfCompare
  suspicious/noAsyncPromiseExecutor
  suspicious/useGetterReturn
  security/noDangerouslySetInnerHtml
  style/useNodejsImportProtocol
  a11y/noDistractingElements
  nursery/useFocusableInteractive
  complexity/noWith
  suspicious/noArrayIndexKey
  nursery/noInvalidDirectionInLinearGradient
  suspicious/noDuplicateClassMembers
  complexity/noExtraBooleanCast
  performance/noAccumulatingSpread
  a11y/useValidAriaProps
  a11y/noRedundantAlt
  correctness/noChildrenProp
  suspicious/noConfusingLabels
  nursery/useSemanticElements
  nursery/useGenericFontNames
  nursery/noDuplicateSelectorsKeyframeBlock
  suspicious/noConfusingVoidType
  a11y/useButtonType
  suspicious/noFocusedTests
  a11y/noAriaUnsupportedElements
  correctness/noFlatMapIdentity
  a11y/noBlankTarget
  a11y/useHeadingContent
  correctness/useValidForDirection
  nursery/noDoneCallback
  correctness/noVoidTypeReturn
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  a11y/useAnchorContent
  complexity/noUselessRename
  correctness/noInvalidNewBuiltin
  style/useNumberNamespace
  complexity/noUselessConstructor
  a11y/noAccessKey
  style/useExponentiationOperator
  style/noUnusedTemplateLiteral
  complexity/noUselessSwitchCase
  nursery/noUnknownUnit
  style/useSingleVarDeclarator
  suspicious/noExportsInTest
  a11y/noNoninteractiveElementToInteractiveRole
  style/noCommaOperator
  nursery/noUnknownProperty
  suspicious/useIsArray
  a11y/noHeaderScope
  complexity/noUselessFragments
  nursery/noDuplicateAtImportRules
  complexity/noForEach
  suspicious/noClassAssign
  suspicious/noMisrefactoredShorthandAssign
  suspicious/noFunctionAssign

Workspace:
  Open Documents:               0

Rule name

lint/complexity/noForEach

Playground link

https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAB7ACAARQBmAGYAZQBjAHQALAAgAFAAcgBlAGQAaQBjAGEAdABlACwAIABwAGkAcABlACAAfQAgAGYAcgBvAG0AIAAnAGUAZgBmAGUAYwB0ACcAOwAKAAoAYwBvAG4AcwB0ACAAYQByAHIAYQB5ACAAPQAgAFsAMQAsACAAMgAsACAAbgB1AGwAbAAsACAAMwBdADsAIAAKAAoAcABpAHAAZQAoAAoAIAAgAGEAcgByAGEAeQAsAAoAIAAgAEUAZgBmAGUAYwB0AC4AZgBvAHIARQBhAGMAaAAoAHgAIAA9AD4AIABQAHIAZQBkAGkAYwBhAHQAZQAuAGkAcwBOAG8AdABOAHUAbABsAGEAYgBsAGUAKAB4ACkALAAgAHsAIABjAG8AbgBjAHUAcgByAGUAbgBjAHkAOgAgACcAdQBuAGIAbwB1AG4AZABlAGQAJwAgAH0AKQAKACkACgA%3D

Expected result

The lint rule lint/complexity/noForEach is very useful for plain arrays, and I would like to enable it.

I read the caveat on the rule page. In our codebase however, we use a lot of Effect, a functional programming library. Effect.forEach is used often enough that it would be bothersome to add biome-ignore comments to every usage.

Would it be feasible to make this rule configurable to ignore some objects?

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Sec-ant Sec-ant added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug S-Needs discussion Status: needs a discussion to understand criteria S-Enhancement Status: Improve an existing feature and removed S-Bug-confirmed Status: report has been confirmed as a valid bug labels Jul 8, 2024
@Sec-ant
Copy link
Member

Sec-ant commented Jul 8, 2024

Would it be feasible to make this rule configurable to ignore some objects?

I personally have no objection to that but would like to know how others see it. We had this similar issue in #1205, but the Effect lib is clearly a more convincing use case.

@ematipico
Copy link
Member

I don't know, to be honest. Providing an option for this rule is like cheating, a user could use the option to bypass some code, e.g.

const arr = [];
arr.forEach();

Then, add arr to the options. For us, options should be a way to empower the rule, not a way to "cheat". I think the correct course of action is for users to suppress of turn off the rule.

@riordanpawley
Copy link

I don't agree that lint rules are for policing, that's for pull requests. Lint rules are guidelines that can be avoided in many ways already. Adding an option, such as the suggestion above, allows apis such as Effect to work within the rules instead of forcing people to turn them off.
If a pull request to your project included an adjustment to the biome lint rules; turning off a rule for variables called "arr" - that is a red flag, just as a biome ignore comment would be.

Note: I have skin in the game as I use effect. I don't want to turn off a perfectly good linting rule if possible.

@ematipico
Copy link
Member

ematipico commented Aug 9, 2024

How would a configuration would look like?

@nickrttn
Copy link
Author

nickrttn commented Oct 1, 2024

Maybe something like the below would work?

{
  "noForEach": {
    "level": "error",
    "options": {
      "ignore": ["Effect.forEach"]
    }
}

@Conaclos
Copy link
Member

Conaclos commented Oct 1, 2024

@nickrttn Why not disable the rule?

@nickrttn
Copy link
Author

nickrttn commented Oct 1, 2024

Because it does have value to us outside of Effect.

@ematipico
Copy link
Member

PRs are welcome, here's an acceptable option:

{
  "noForEach": {
    "level": "error",
    "options": {
      "validIdentifiers": ["Effect", "_"]
    }
}

The rule will check if forEach is called by an expression that matches Effect or _. e.g.

Effect.forEach();
_.forEach();

Values with dots won't be accepted. A value such as "something._" won't work.

@ematipico ematipico added good first issue Good for newcomers S-Help-wanted Status: you're familiar with the code base and want to help the project and removed S-Needs discussion Status: needs a discussion to understand criteria labels Oct 7, 2024
@lucasweng lucasweng linked a pull request Nov 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter good first issue Good for newcomers L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants