-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Lint #4276
Lint #4276
Changes from 8 commits
d138202
7e4e39d
8f10741
bb28903
d5a06d1
3ca191d
45242b1
a0e618e
02c9672
0075d18
55cb73a
5bc6a8b
1d04c6e
3d657a1
44d1aa6
b0b3166
3461b7e
e2314fe
60de2e8
84f530e
4000ffa
72bd7f2
0de745d
1d1fb09
b8bee86
ee7a3cf
228ec96
265c8fa
3116544
32af8df
b775d52
a2cf78c
d97d253
d2d5c0d
2215257
c2b4f6d
d83acea
37b5734
43c007a
05cf4ad
3761225
22b4592
0c93073
a87d50d
db29dba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ function getSingleButton(prNumber: number, _?: number, prs?: number[]): HTMLElem | |
const getPrsByFile = cache.function(async (): Promise<Record<string, number[]>> => { | ||
const {repository} = await api.v4(` | ||
repository() { | ||
pullRequests( | ||
pullRequests ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added on purpose it looked wrong without a space and the arguments on a new line. I can revert if you feel it wrong There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't do this anywhere in the code and also doesn't match the function call style in XO. Reverted |
||
first: 25, | ||
states: OPEN, | ||
baseRefName: "${await getDefaultBranch()}", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem again? It worked until now and typescript-eslint/typescript-eslint#1976 was resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue, XO re enabled the rule now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Sindre re-enabled them they're probably working now, so maybe our code is wrong.
The only reason I can think of to disable these are if the changes required just clutter the code without actually improving it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@typescript-eslint/no-non-null-assertion
is correct in being disabled, we need those errors.I'm checking the other one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@typescript-eslint/no-non-null-asserted-optional-chain
should probably not be disabled.However I'm not liking most of the type changes in this PR so I'd rather avoid all of them until a TS developer can review them properly.
If you're going to update XO:
no-unsafe-argument
+ a TODO to review laterno-non-null-asserted-optional-chain
+ same TODOno-non-null-assertion
with a comment that we need those runtime errorsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fregante in case you missed this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find a JSON to YML converter and rename the file to
.yml
instead of.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That did not work.
I am going to revert the Type's and just update dep excluding xo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I assumed it worked since ESLint supports it: xojs/xo#536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not entirely true: https://github.com/sindresorhus/sindre-playground/blob/565ba9c61dbf4b13d587bcae7047120952f924c5/package.json#L26-L31