Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyzer): promote nursery rules #4239

Merged
merged 10 commits into from
Mar 4, 2023
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Feb 25, 2023

Summary

This PR aims to promote the nursery rules that were shipped since 11.0.0 version. Here's a breakdown of the changes:

suspicious

  • noEmptyInterface
  • noExtraNonNullAssertion
  • noRedundantUseStrict
  • noConstEnum
  • useDefaultSwitchClauseLast

correctness

  • noUnsafeFinally
  • noDuplicateObjectKeys
  • noConstructorReturn
  • noPrecisionLoss
  • noVoidTypeReturn
  • noStringCaseMismatch
  • noSetterReturn

style

  • useExponentiationOperator
  • useNumericLiterals
  • useDefaultParameterLast
  • useConst
  • noVar
  • useEnumInitializers
  • noNonNullAssertion

a11y

  • noDistractingElements
  • noHeaderScope
  • noAccessKey

complexity

  • useSimplifiedLogicExpression not recommended

Open questions

Rules noVar and useConst are a bit controversial. I needed to figure out where to put them.
They don't catch any bug, so correctness. People can use var all over their code base, which should be fine.

I don't want Rome to get in the way of developers too much.

I am happy to have some feedback and ear opinions!

Test Plan

CI should pass

Documentation

Not needed, everything is autogenerated.

@netlify
Copy link

netlify bot commented Feb 25, 2023

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 38000ff
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/640338ce9aea3b0008be4af6

@ematipico ematipico force-pushed the chore/promote-rules branch 2 times, most recently from ca070f3 to d4c356f Compare March 1, 2023 10:13
@ematipico ematipico marked this pull request as ready for review March 1, 2023 10:13
@ematipico ematipico requested a review from Conaclos March 1, 2023 10:13
Copy link
Contributor

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions:

  • Because noDuplicateParameters is in suspicious, I think that noDuplicateObjectKeys should also be in suspicious.

  • Because noExtraNonNullAssertion is part of suspicious, we could move noNonNullAssertion in style.

  • I could move noSetterReturn to correctness.

  • In contrast, I could move noConstructorReturn to suspicious because there are non-idiomatic use of return in a constructor.

  • I could move useEnumInitializers and noConstEnum to style.

Rules noVar and useConst are a bit controversial. I needed to figure out where to put them.
They don't catch any bug, so correctness. People can use var all over their code base, which should be fine.

I think style is a good place for them :)

I don't want Rome to get in the way of developers too much.

This means that these rules are not in recommended?

By the way, I am wondering if we should un-promote some rules that are known as unstable (i.e. move them to nursery). I am thinking about useOptionalChain #4145 and useSimplifiedLogicExpression #4163.

@Conaclos
Copy link
Contributor

Conaclos commented Mar 1, 2023

Regarding noVar and useConst, I am wondering if we should introduce a kind of strict mode that supersedes recommended. This is the approach taken by ESlint TypeScript to provide more opinionated rules.

@ematipico
Copy link
Contributor Author

@Conaclos Thank you for all your suggestions! They all make sense! Here some question for you:

I could move useEnumInitializers and noConstEnum to style.

Any particular reason why? It makes sense for useEnumInitializers, but noConstEnum has a description that makes me think "OK, maybe something could go wrong...".

This means that these rules are not in recommended?

They are recommended. They just go under a category that forces how to style your code.

By the way, I am wondering if we should un-promote some rules that are known as unstable (i.e. move them to nursery). I am thinking about useOptionalChain #4145 and useSimplifiedLogicExpression #4163.

  • useSimplifiedLogicExpression => we could not recommend it;
  • useOptionalChain => I am not sure about it... it's a useful rule, and people could turn it off via comment if they need; 🤔

@Conaclos
Copy link
Contributor

Conaclos commented Mar 1, 2023

noConstEnum has a description that makes me think "OK, maybe something could go wrong...".

You are right. Let's keep it in suspicious.

useOptionalChain => I am not sure about it... it's a useful rule, and people could turn it off via comment if they need; thinking

I think it is a good rule to have and to recommend. I proposed to temporary move the rule to nursery because of the reported issues. Another solution could be to fix most of its issues before the release. Not sure how to proceed...

@ematipico
Copy link
Contributor Author

Yeah, let's see if we can flash out a fix for it. I think it's worth it!

@ematipico
Copy link
Contributor Author

I will merge it; if you have any concerns or proposals, feel free to comment or open a PR to update the promotion of the rules.

@ematipico ematipico merged commit dc6e4ea into main Mar 4, 2023
@ematipico ematipico deleted the chore/promote-rules branch March 4, 2023 18:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants