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

fix: fix eslint-plugin runtime import check rule #10667

Conversation

ohmyide
Copy link
Contributor

@ohmyide ohmyide commented Jan 25, 2022

What it does

fix eslint-plugin runtime import check rule

How to test

run: yarn lint

case:
in a common folder like: src/common/**.ts ,the rule only matched /browser/, unmatched /browser;:

import { FrontendApplicationContribution } from '@theia/core/lib/browser'; 

if (matchedImportRule.restricted.some(restricted => module.includes(/${restricted}/))) {

Review checklist

Reminder for reviewers

Signed-off-by: ohmyide [email protected]

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @ohmyide 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.


Can you describe a use-case that the rule fails today, it was not clear in your description.

@vince-fugnitto vince-fugnitto added the linting issues related to linting label Jan 25, 2022
@ohmyide ohmyide force-pushed the fix/eslint-plugin-runtime-import-check branch 2 times, most recently from 8ce5ca9 to 4cec0e3 Compare January 26, 2022 04:11
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good, it looks like you were able to identify another issue in the codebase.
We will need to suppress it for the time being so we can then fix it (error).

You can suppress it like other instances, using:

packages/plugin-ext/src/common/plugin-api-rpc.ts

// eslint-disable-next-line @theia/runtime-import-check
import { PickOptions, QuickInputButtonHandle, QuickPickItem, WidgetOpenerOptions } from '@theia/core/lib/browser';

@vince-fugnitto vince-fugnitto self-requested a review January 26, 2022 13:17
@ohmyide
Copy link
Contributor Author

ohmyide commented Jan 26, 2022

Sorry, I am late.

we knows that: common dir only can import common file, the runtime-import-check can help us find the error import.
But The rule in the source code only matched /browser/, in the case like: /browser , it can't matched (it is not end of /).

in my project:
image
The '@theia/core/lib/browser' can't import in common dir, runtime-import-check should find it, and give me a lint error, but it does not work, because it is not end of '/browser/'.

I think it's a rule mistake, so I try to fix it.

I'm not good at English, I don't know if I made it clear.

@vince-fugnitto
Copy link
Member

@ohmyide I understand the fix :) I confirmed that it works and you actually helped identify an issue which would need to be suppressed for the time being like I commented in #10667 (review) (else the CI checks fail) 👍

@ohmyide
Copy link
Contributor Author

ohmyide commented Jan 26, 2022

@ohmyide I understand the fix :) I confirmed that it works and you actually helped identify an issue which would need to be suppressed for the time being like I commented in #10667 (review) (else the CI checks fail) 👍

ok, thanks for your prompt reply

@vince-fugnitto
Copy link
Member

@ohmyide do you mind rebasing the pull-request and making the change I mentioned here? If you need any help please let me know! :)

@ohmyide ohmyide force-pushed the fix/eslint-plugin-runtime-import-check branch 2 times, most recently from a1c2e0b to bf908bb Compare February 8, 2022 13:21
@ohmyide
Copy link
Contributor Author

ohmyide commented Feb 8, 2022

@vince-fugnitto
done.
I add an eslint-disable-next-line in plugin-ext/src/common/plugin-api-rpc.ts

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me, thank you for your contribution 👍
@paul-marechal do you have any objections?

@ohmyide ohmyide force-pushed the fix/eslint-plugin-runtime-import-check branch from bf908bb to ffebaf6 Compare February 11, 2022 19:23
…ble-next-line in plugin-api-rpc.ts

Signed-off-by: chuanjin wang <[email protected]>
@ohmyide ohmyide force-pushed the fix/eslint-plugin-runtime-import-check branch from ffebaf6 to 9e56bfa Compare February 11, 2022 19:25
@paul-marechal paul-marechal merged commit bc16bf7 into eclipse-theia:master Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linting issues related to linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants