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

Lint #4276

Merged
merged 45 commits into from May 6, 2021
Merged

Lint #4276

merged 45 commits into from May 6, 2021

Conversation

yakov116
Copy link
Member

@yakov116 yakov116 commented Apr 25, 2021

Disabled "@typescript-eslint/no-non-null-assertion": not compatible with DOM
Disabled "@typescript-eslint/no-non-null-asserted-optional-chain" See #3093 (comment) and #3168 (comment)

Need typescript help
@typescript-eslint/no-unsafe-argument All but 2 are this issue. If you show me how to fix one I can do the rest.

Disabled "@typescript-eslint/no-non-null-assertion": not compatible with DOM
Disabled " @typescript-eslint/no-non-null-asserted-optional-chain"  See #3093 (comment) and #3168 (comment)
@yakov116 yakov116 requested a review from fregante April 25, 2021 12:52
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

What fixed .closest’s types?

source/features/no-duplicate-list-update-time.tsx Outdated Show resolved Hide resolved
source/options.tsx Outdated Show resolved Hide resolved
source/features/file-finder-buffer.tsx Outdated Show resolved Hide resolved
.xo-config.json Outdated
@@ -16,6 +16,8 @@
"no-void": "off",
"react/jsx-key": "off",
"unicorn/no-array-callback-reference": "off",
"@typescript-eslint/no-non-null-assertion": "off",
"@typescript-eslint/no-non-null-asserted-optional-chain": "off",
Copy link
Member

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

Copy link
Member Author

@yakov116 yakov116 Apr 25, 2021

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.

Disabled "@typescript-eslint/no-non-null-asserted-optional-chain" See #3093 (comment) and #3168 (comment)

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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:

  • disable no-unsafe-argument + a TODO to review later
  • disable no-non-null-asserted-optional-chain + same TODO
  • disable no-non-null-assertion with a comment that we need those runtime errors

Copy link
Member Author

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

Copy link
Member

@fregante fregante May 4, 2021

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

Copy link
Member Author

@yakov116 yakov116 May 4, 2021

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

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

We are using a JSON file which makes it not possible to leave a comment.

That's not entirely true: https://github.com/sindresorhus/sindre-playground/blob/565ba9c61dbf4b13d587bcae7047120952f924c5/package.json#L26-L31

@fregante
Copy link
Member

fregante commented Apr 25, 2021

Need typescript help

There are a million type issues, can't open a PR and ask me to fix them. Just leave things unchanged

@fregante fregante added the meta Related to Refined GitHub itself label Apr 25, 2021
@yakov116
Copy link
Member Author

There are a million type issues, can't open a PR and ask me to fix them. Just leave things unchanged

@typescript-eslint/no-unsafe-argument All but 2 are this issue. If you show me how to fix one I can do the rest.

Sorry I wanted to update the first post I did not think you would review so fast 😄

@yakov116
Copy link
Member Author

What fixed .closest’s types

Its not fixed.

@fregante
Copy link
Member

What fixed .closest’s types

Its not fixed.

I see that <HTMLElement> is being dropped from all. Why? If it works without, that's what I mean by "fixed"

@yakov116
Copy link
Member Author

I see that <HTMLElement> is being dropped from all. Why? If it works without, that's what I mean by "fixed"

I am going to work on it. That part is not ready yet.

@fregante
Copy link
Member

If this PR makes it for May 1st, can you revert #4222 in here due to #4275?

@yakov116
Copy link
Member Author

If this PR makes it for May 1st, can you revert #4222 in here due to #4275?

Sure

@fregante
Copy link
Member

I wish could could change some global types, but adding this to global.d.ts doesn't work 😩

interface Object {
	keysa<T, K extends keyof T>(o: T): K[];
}

interface JSON {
	parse(text: string, reviver?: (this: any, key: string, value: any) => any): JsonValue;
}

// and also Response#json()

.xo-config.json Outdated Show resolved Hide resolved
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Other than the outstanding reviews, it looks good so far, except I haven't tested it

source/features/release-download-count.tsx Outdated Show resolved Hide resolved
source/globals.d.ts Outdated Show resolved Hide resolved
@@ -61,7 +61,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 (
Copy link
Member

Choose a reason for hiding this comment

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

^

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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

@yakov116 yakov116 marked this pull request as ready for review May 6, 2021 02:51
@yakov116
Copy link
Member Author

yakov116 commented May 6, 2021

@fregante can we disable clean-conversation-sidebar in this PR?

@fregante
Copy link
Member

fregante commented May 6, 2021

Ok

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Should be good to go after this

source/features/release-download-count.tsx Show resolved Hide resolved
@yakov116
Copy link
Member Author

yakov116 commented May 6, 2021

@fregante
Copy link
Member

fregante commented May 6, 2021

Second bullet point in that PR #3678

Just comment out the import clean-conversation-sidebar line in refined-github.ts. It might be worth adding this piece of information at the top of the file.

@fregante fregante merged commit 2fc61cf into main May 6, 2021
@fregante fregante deleted the lint branch May 6, 2021 07:39
fregante added a commit that referenced this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to Refined GitHub itself
Development

Successfully merging this pull request may close these issues.

None yet

3 participants