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

Use typescript recommended-type-checked rules #12

Merged
merged 3 commits into from
Dec 22, 2023

Conversation

nkuba
Copy link
Member

@nkuba nkuba commented Dec 21, 2023

We switch from @typescript-eslint/recommended to @typescript-eslint/recommended-type-checked.
The new configuration extends the previous one with additional rules related to type information.

We switch from [`@typescript-eslint/recommended`](https://typescript-eslint.io/linting/configs/#recommended)
to
[`@typescript-eslint/recommended-type-checked`](https://typescript-eslint.io/linting/configs/#recommended-type-checked).
The new configuration extends the previous one with additonal rules
related to types checking.
We disable the `plugin:@typescript-eslint/recommended-type-checked"`
rules for JS files as recommended in https://typescript-eslint.io/linting/typed-linting#how-can-i-disable-type-aware-linting-for-a-subset-of-files
@nkuba nkuba self-assigned this Dec 21, 2023
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do we have a reference PR with the kinds of changes this might trigger in code we've written? Will merge in the meantime as this is a -pre version anyway.

@Shadowfiend Shadowfiend merged commit 7b9bc8c into main Dec 22, 2023
1 check passed
@Shadowfiend Shadowfiend deleted the typescript-rules-type-checked branch December 22, 2023 17:42
nkuba added a commit to thesis/acre that referenced this pull request Dec 28, 2023
This version enables `@typescript-eslint/recommended-type-checked`
rules.
The commit hash refers to the merge commit of
thesis/eslint-config#12.
nkuba added a commit to thesis/acre that referenced this pull request Dec 28, 2023
This version enables `@typescript-eslint/recommended-type-checked`
rules.
The commit hash refers to the merge commit of
thesis/eslint-config#12.
nkuba added a commit to thesis/acre that referenced this pull request Dec 28, 2023
This version enables `@typescript-eslint/recommended-type-checked`
rules.
The commit hash refers to the merge commit of
thesis/eslint-config#12.
@nkuba
Copy link
Member Author

nkuba commented Dec 28, 2023

Do we have a reference PR with the kinds of changes this might trigger in code we've written?

Please find the reference PR here: thesis/acre#98

r-czajkowski added a commit to thesis/acre that referenced this pull request Jan 4, 2024
Update `@thesis-co/eslint-config` to the latest version. The commit hash
refers to the merge commit of
thesis/eslint-config#12. This version enables
`@typescript-eslint/recommended-type-checked` rules.

The configuration enforces multiple typing check rules that I found very
useful in the core module's typescript tests, where we were missing
`await` in a couple of places.

Example:
```ts
        it("should emit StakeReferral event", () => {
          expect(tx)
            .to.emit(acre, "StakeReferral")
            .withArgs(referral, amountToStake)
        })
```
Without the `await` the test returned a false-positive result and hadn't
checked if expect resolves correctly.

⚠️ Typing checks in the Core module require `typechain/` directory to be
generated before eslint verification is run. It is recommended that on
local environments `pnpm run build` is run before `pnpm run format`. In
this PR we update the CI process accordingly.

This PR updates the configuration of the Core, SDK, and Website modules.
This PR doesn't update the configuration of the dApp module, we should
handle that in #99.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants