[eslint] Tweak ESLint rules for commonly ignored cases #2010
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rationale: - `complexity` - Seems very low, very common to have large React components where breaking them into separate modules only leads to prop-drilling and unnecessarily deep trees. Also quite common to have readable but long validation/switch statements where adjusting to standard only makes code less readable rather than more. Depend on code reviews to catch hard-to-read code. If 30 is not enough for actual cases, ignore in-place. - `id-length` - `itemA`/`itemB` and similar is not necessarily more readable than `a`, `b`. Lots of exceptions already in place. Commonly disabled. Catch in code reviews if variables are difficultly named. - `max-depth` - Seems arbitrarily low. Quite common to have valid, deeper blocks than this. Again, catch exceptions in code-reviews rather than linting. - `max-len` - Tab size seems to be set wrong. Have not seen this play out anywhere, but tweaked for consistency. - `no-await-in-loop` - Lots of cases where this is useful and "correct" (retrying, for instance). Catch in code reviews if using it in cases where `Promise.all` or a concurrency limiter should be in place. - `@typescript-eslint/explicit-module-boundary-types` - disable for tsx because defining `React.ReactElement` as return value is annoying. Still a good rule to ensure return values are what you expect in most other cases, however. - `react/prop-types` - Disable for typescript because we want types, not runtime prop checks - `react/require-default-props` - Disable for typescript because we want types, not runtime prop checks Also exposed an `eslint-config-sanity/typescript` preset for easy use in other projects
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sanity-io/test-studio/7u40j3rcp |
bjoerge
approved these changes
Sep 16, 2020
|
Nice work! |
rexxars
added a commit
that referenced
this pull request
Sep 22, 2020
Rationale: - `complexity` - Seems very low, very common to have large React components where breaking them into separate modules only leads to prop-drilling and unnecessarily deep trees. Also quite common to have readable but long validation/switch statements where adjusting to standard only makes code less readable rather than more. Depend on code reviews to catch hard-to-read code. If 30 is not enough for actual cases, ignore in-place. - `id-length` - `itemA`/`itemB` and similar is not necessarily more readable than `a`, `b`. Lots of exceptions already in place. Commonly disabled. Catch in code reviews if variables are difficultly named. - `max-depth` - Seems arbitrarily low. Quite common to have valid, deeper blocks than this. Again, catch exceptions in code-reviews rather than linting. - `max-len` - Tab size seems to be set wrong. Have not seen this play out anywhere, but tweaked for consistency. - `no-await-in-loop` - Lots of cases where this is useful and "correct" (retrying, for instance). Catch in code reviews if using it in cases where `Promise.all` or a concurrency limiter should be in place. - `@typescript-eslint/explicit-module-boundary-types` - disable for tsx because defining `React.ReactElement` as return value is annoying. Still a good rule to ensure return values are what you expect in most other cases, however. - `react/prop-types` - Disable for typescript because we want types, not runtime prop checks - `react/require-default-props` - Disable for typescript because we want types, not runtime prop checks Also exposed an `eslint-config-sanity/typescript` preset for easy use in other projects
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for rule changes:
complexity- Seems very low, very common to have large React components where breaking them into separate modules only leads to prop-drilling and unnecessarily deep trees. Also quite common to have readable but long validation/switch statements where adjusting to standard only makes code less readable rather than more. Depend on code reviews to catch hard-to-read code. If 30 is not enough for actual cases, ignore in-place.id-length-itemA/itemBand similar is not necessarily more readable thana,b. Lots of exceptions already in place. Commonly disabled. Catch in code reviews if variables are difficultly named.max-depth- Seems arbitrarily low. Quite common to have valid, deeper blocks than this. Again, catch exceptions in code-reviews rather than linting.max-len- Tab size seems to be set wrong. Have not seen this play out anywhere, but tweaked for consistency.no-await-in-loop- Lots of cases where this is useful and "correct" (retrying, for instance). Catch in code reviews if using it in cases wherePromise.allor a concurrency limiter should be in place.@typescript-eslint/explicit-module-boundary-types- disable for tsx because definingReact.ReactElementas return value is annoying. Still a good rule to ensure return values are what you expect in most other cases, however.react/prop-types- Disable for typescript because we want types, not runtime prop checksreact/require-default-props- Disable for typescript because we want types, not runtime prop checksAlso exposed an
eslint-config-sanity/typescriptpreset for easy use in other projects