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

Update @thesis-co/eslint-config to the latest 0.8.0-pre version #98

Merged
merged 11 commits into from
Jan 4, 2024

Conversation

nkuba
Copy link
Member

@nkuba nkuba commented Dec 28, 2023

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:

        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.

When using ethers we should import it from hardhat.
This version enables `@typescript-eslint/recommended-type-checked`
rules.
The commit hash refers to the merge commit of
thesis/eslint-config#12.
@nkuba nkuba force-pushed the eslint-ts-recommended branch from cc567ea to 58576bd Compare December 28, 2023 21:01
The problems were reported after the latest upgrade of eslint-config.
Since the eslint rules we enabled for typescript with the latest
eslint-config upgrade check typings, we need to ensure that `typechain/`
directory is generated before we run eslint.
For this reason we first run build and later format.
@nkuba nkuba self-assigned this Dec 28, 2023
@nkuba nkuba added the ⚙️ Backend APIs, Maintainers label Dec 28, 2023
@nkuba nkuba changed the title Update @thesis-co/eslint-config to the latest version Update @thesis-co/eslint-config to the latest 0.8.0-pre version Dec 28, 2023
Copy link
Contributor

@r-czajkowski r-czajkowski left a comment

Choose a reason for hiding this comment

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

Left 2 non-blocking comments. Overall LGTM 👍

core/deploy/21_transfer_ownership_acre.ts Outdated Show resolved Hide resolved
core/deploy/22_transfer_ownership_acre_router.ts Outdated Show resolved Hide resolved
nkuba added 2 commits January 3, 2024 12:50
Instead of adding inline comments for unbound-method rule we define it
in the eslint config, as the `const { log } = deployments` is and will
be broadly used in the scripts.
@nkuba nkuba requested a review from r-czajkowski January 3, 2024 11:59
core/.eslintrc Outdated
Comment on lines 17 to 24
"overrides": [
{
"files": ["deploy/*.ts"],
"rules": {
"@typescript-eslint/unbound-method": "off"
}
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are touching the linting config I thought we could also override @typescript-eslint/no-unused-expressions for test files to use to.be.true/false. Otherwise, we need to use to.be.equal(true) see https://github.com/thesis/acre/blob/main/core/test/Dispatcher.test.ts#L68.

{
      "files": ["*.test.ts"],
      "rules": {
        "@typescript-eslint/no-unused-expressions": "off"
      }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

r-czajkowski
r-czajkowski previously approved these changes Jan 3, 2024
We want to use `to.be.true` instead of `to.be.equal(true)`, so we need
to disable the rule.
nkuba added 3 commits January 4, 2024 09:36
  26:70  error  Async arrow function has no 'await' expression  @typescript-eslint/require-await
This fixes:
 Error:   861:11  error  Unsafe assignment of an `any` value                 @typescript-eslint/no-unsafe-assignment
Error:   861:33  error  Unsafe call of an `any` typed value                 @typescript-eslint/no-unsafe-call
Error:   861:33  error  Unsafe call of an `any` typed value                 @typescript-eslint/no-unsafe-call
Error:   861:40  error  Unsafe member access .Wallet on an `any` value      @typescript-eslint/no-unsafe-member-access
Error:   861:62  error  Unsafe member access .getAddress on an `any` value  @typescript-eslint/no-unsafe-member-access
@nkuba nkuba requested a review from r-czajkowski January 4, 2024 08:55
@r-czajkowski r-czajkowski enabled auto-merge January 4, 2024 09:43
@r-czajkowski r-czajkowski merged commit 803140d into main Jan 4, 2024
11 checks passed
@r-czajkowski r-czajkowski deleted the eslint-ts-recommended branch January 4, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ Backend APIs, Maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants