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.

Sign up for GitHub

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

Thesis - eslint config #143

Merged
merged 7 commits into from
Jan 11, 2024
Merged

Thesis - eslint config #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f7419d
Thesis - eslint config
ioay Jan 9, 2024
1c5a960
Added eslint rules
ioay Jan 9, 2024
1cf2309
Added comments to eslint config
ioay Jan 10, 2024
25884ef
Added new eslint rule related with the last #105 changes
ioay Jan 10, 2024
bc983be
Removal of global exclusion rules, added temporary overrides rules fo…
ioay Jan 11, 2024
e79848e
Merge branch 'main' into eslint-config-thesis
ioay Jan 11, 2024
f903ed1
Eslint rules extension after last changes on main branch
ioay Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 58 additions & 1 deletion dapp/.eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,65 @@
2,
{ "allowRequiredDefaults": true }
],
"react/require-default-props": [0]
"react/require-default-props": [0],
},
// FIXME:
// This is temporary solution after changes of the eslint-config version: @thesis-co/eslint-config: "github:thesis/eslint-config#7b9bc8c"
// Overrides rules should be fixed file by file.
"overrides": [
{
"files": [
"src/components/Header/ConnectWallet.tsx",
"src/components/Modals/Support/MissingAccount.tsx",
"src/components/Modals/Staking/SignMessage.tsx",
"src/hooks/useDepositBTCTransaction.ts",
"src/components/shared/Form/FormTokenBalanceInput.tsx"
],
"rules": {
"@typescript-eslint/no-misused-promises": "off"
}
},
{
"files": [
"src/hooks/useSignMessage.ts"
],
"rules": {
"@typescript-eslint/no-floating-promises": "off"
}
},
{
"files": [
"src/theme/*"
],
"rules": {
"@typescript-eslint/unbound-method": "off"
}
},
{
"files": [
"src/theme/Alert.ts"
],
"rules": {
"@typescript-eslint/no-unsafe-member-access": "off"
}
},
{
"files": [
"src/components/shared/Form/FormTokenBalanceInput.tsx"
],
"rules": {
"@typescript-eslint/no-unsafe-assignment": "off"
}
},
{
"files": [
"src/components/shared/TokenAmountForm/index.tsx"
],
"rules": {
"@typescript-eslint/require-await": "off"
}
}
],
Comment on lines +20 to +76
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just one additional note here: in the future, it's probably better to do these directly in the file via /* eslint-ignore */ or // eslint-ignore-next comments. The core reasoning is that this leaves them in your face whenever you interact with those files, vs having them tucked away in an eslintrc that will likely be looked at once every 6 months at the most.

Glad we scoped this down a good bit regardless, thanks y'all!

Copy link
Member

Choose a reason for hiding this comment

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

We want to revisit all of the ignores in a follow-up PR as soon as possible (hence the FIXME and referenced issue not closed yet #99).
I was afraid that leaving the comments inline the files may make it harder to distinguish which ignores are intentional and which we want to revisit and fix.

I agree 💯 inline disables should be preferred over the ones in eslintrc.

Thank you for chiming in sir! 🙇🏻

Copy link
Collaborator

@Shadowfiend Shadowfiend Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also I definitely meant eslint-disable*.

Even with inline disables I always encourage reasoning, so dropping a FIXME on it is def compatible. For example -> https://github.com/tahowallet/extension/blob/aa5c3b6ff3cee162314d473938d62aad41555b28/background/redux-slices/activities.ts#L1-L3, https://github.com/tahowallet/extension/blob/aa5c3b6ff3cee162314d473938d62aad41555b28/ui/components/Signing/Signer/SignerFrame.tsx#L9C1-L13 or https://github.com/tahowallet/extension/blob/aa5c3b6ff3cee162314d473938d62aad41555b28/ui/components/Shared/SharedIconRouterLink.tsx#L19-L20 (though the latter is arguably a bad example since we never completed the todo 😆).

Again though, this is really about future stuff, and I hear you re: the open issue. Nothing is ever perfect!

"settings": {
"import/resolver": {
"alias": {
Expand Down
2 changes: 1 addition & 1 deletion dapp/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"react-number-format": "^5.3.1"
},
"devDependencies": {
"@thesis-co/eslint-config": "^0.6.1",
"@thesis-co/eslint-config": "github:thesis/eslint-config#7b9bc8c",
"@types/react": "^18.2.38",
"@types/react-dom": "^18.2.17",
"@typescript-eslint/eslint-plugin": "^6.12.0",
Expand Down
4 changes: 2 additions & 2 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.