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

feat(linter): eslint/max-len #2874

Merged
merged 34 commits into from
Apr 19, 2024
Merged

feat(linter): eslint/max-len #2874

merged 34 commits into from
Apr 19, 2024

Conversation

woai3c
Copy link
Contributor

@woai3c woai3c commented Mar 30, 2024

No description provided.

@github-actions github-actions bot added the A-linter Area - Linter label Mar 30, 2024
@Dunqing Dunqing requested a review from mysteryven March 31, 2024 02:04
crates/oxc_linter/src/rules/eslint/max_len.rs Show resolved Hide resolved
crates/oxc_linter/src/rules/eslint/max_len.rs Outdated Show resolved Hide resolved
crates/oxc_linter/src/rules/eslint/max_len.rs Outdated Show resolved Hide resolved
@mysteryven mysteryven marked this pull request as draft April 6, 2024 13:53
@mysteryven
Copy link
Contributor

Change it to draft because of not ready yet. 😉

@woai3c
Copy link
Contributor Author

woai3c commented Apr 6, 2024

Change it to draft because of not ready yet. 😉

Okay, I'm currently preparing for a job search and I don't have much time. I'm sorry.

@woai3c woai3c closed this Apr 9, 2024
@mysteryven
Copy link
Contributor

If you are busy but still want to contribute to our repo, this issue may more suitable for you than this rule.🤔

@woai3c
Copy link
Contributor Author

woai3c commented Apr 10, 2024

If you are busy but still want to contribute to our repo, this issue may more suitable for you than this rule.🤔

If you are busy but still want to contribute to our repo, this issue may more suitable for you than this rule.🤔

Thank you very much for your reply. I really want to contribute to the code, so thank you. I closed this PR because I found that the workload involved is quite large, and there are some issues I don't understand and can't find someone to solve, so I want to close it first. When I was implementing the max-len rule, I encountered two problems: 1. The fn from_configuration() method that each rule has should merge the default values and the configuration values, but I can't find where it is executed. 2. It is necessary to use AST to parse the code, such as determining comments. Can I directly use the AST of oxc to scan here?

@woai3c
Copy link
Contributor Author

woai3c commented Apr 10, 2024

If you are busy but still want to contribute to our repo, this issue may more suitable for you than this rule.🤔

I'm going to check out the issue you mentioned now.

@woai3c
Copy link
Contributor Author

woai3c commented Apr 10, 2024

If you are busy but still want to contribute to our repo, this issue may more suitable for you than this rule.🤔

If you are busy but still want to contribute to our repo, this issue may more suitable for you than this rule.🤔

Thank you very much for your reply. I really want to contribute to the code, so thank you. I closed this PR because I found that the workload involved is quite large, and there are some issues I don't understand and can't find someone to solve, so I want to close it first. When I was implementing the max-len rule, I encountered two problems:

  1. The fn from_configuration() method that each rule has should merge the default values and the configuration values, but I can't find where it is executed.
  2. It is necessary to use AST to parse the code, such as determining comments. Can I directly use the AST of oxc to scan here?

@mysteryven
Copy link
Contributor

I'm going to check out the issue you mentioned now.

Are you working on it now? The author of this rule(@todor-a) is also planning to do it. My bad, I should have confirmed it first.

@todor-a
Copy link
Contributor

todor-a commented Apr 10, 2024

I'm going to check out the issue you mentioned now.

Are you working on it now? The author of this rule(@todor-a) is also planning to do it. My bad, I should have confirmed it first.

Haven't gotten too far. If you have feel free to finish it!

@woai3c
Copy link
Contributor Author

woai3c commented Apr 10, 2024

I'm going to check out the issue you mentioned now.

Are you working on it now? The author of this rule(@todor-a) is also planning to do it. My bad, I should have confirmed it first.

I haven't started yet, so I'll continue researching max len.

@woai3c
Copy link
Contributor Author

woai3c commented Apr 10, 2024

I'm going to check out the issue you mentioned now.

Are you working on it now? The author of this rule(@todor-a) is also planning to do it. My bad, I should have confirmed it first.

Haven't gotten too far. If you have feel free to finish it!

I haven't started yet, so I'll continue researching max len.

@mysteryven
Copy link
Contributor

  1. The fn from_configuration() method that each rule has should merge the default values and the configuration values, but I can't find where it is executed.

Are you looking for this?

  1. rules_to_replace.push(rule.read_json(rule_config.config.clone()));
    }
  2. pub fn read_json(&self, maybe_value: Option<serde_json::Value>) -> Self {
    match self {
    #(Self::#struct_names(_) => Self::#struct_names(
    maybe_value.map(#struct_names::from_configuration).unwrap_or_default(),
    )),*
    }
    }

It is necessary to use AST to parse the code, such as determining comments. Can I directly use the AST of oxc to scan here?

What's specific problem are you facing?

@woai3c
Copy link
Contributor Author

woai3c commented Apr 10, 2024

  1. The fn from_configuration() method that each rule has should merge the default values and the configuration values, but I can't find where it is executed.

Are you looking for this?

  1. rules_to_replace.push(rule.read_json(rule_config.config.clone()));
    }
  2. pub fn read_json(&self, maybe_value: Option<serde_json::Value>) -> Self {
    match self {
    #(Self::#struct_names(_) => Self::#struct_names(
    maybe_value.map(#struct_names::from_configuration).unwrap_or_default(),
    )),*
    }
    }

It is necessary to use AST to parse the code, such as determining comments. Can I directly use the AST of oxc to scan here?

What's specific problem are you facing?

Thank you for the response. My previous question was about understanding where this rule receives user option values from.

@mysteryven
Copy link
Contributor

where this rule receives user option values from.

config.override_rules(&mut rules, &all_rules);

then it call:

rules_to_replace.push(rule.read_json(rule_config.config.clone()));

The config of this rule are in rule_config.config

rule_config.config are parsing from config file:

let (severity, config) = parse_rule_value(&value).map_err(de::Error::custom)?;

@woai3c woai3c reopened this Apr 11, 2024
@woai3c
Copy link
Contributor Author

woai3c commented Apr 12, 2024

@mysteryven Hi, I have passed all the eslint tests regarding max-len, except for JSX. I'm not sure how to implement it for JSX yet, as the current AST in oxc seems unable to read JSX comments. I need to think about how to approach this. If you have any good suggestions, feel free to advise.

@woai3c woai3c requested a review from mysteryven April 12, 2024 15:48
@woai3c woai3c changed the title feat(oxc_linter): add max_len rule feat(linter): add max_len rule Apr 12, 2024
@woai3c woai3c marked this pull request as ready for review April 13, 2024 02:50
@mysteryven
Copy link
Contributor

It's a really nice work! I need some time to take a close look at it, will give you feedback tomorrow.

@woai3c
Copy link
Contributor Author

woai3c commented Apr 13, 2024

It's a really nice work! I need some time to take a close look at it, will give you feedback tomorrow.

Thank you for your response. If I have some free time in the next few days, I will try to add support for JSX.

Copy link

codspeed-hq bot commented Apr 14, 2024

CodSpeed Performance Report

Merging #2874 will degrade performances by 8.42%

Comparing woai3c:main (e72520f) with main (3f9f467)

Summary

❌ 5 regressions
✅ 25 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main woai3c:main Change
linter[RadixUIAdoptionSection.jsx] 7.2 ms 7.5 ms -4.09%
linter[antd.js] 10 s 10.9 s -7.91%
linter[cal.com.tsx] 2.8 s 3 s -5.76%
linter[checker.ts] 5.1 s 5.6 s -8.42%
linter[pdf.mjs] 1.8 s 1.9 s -4.83%

@mysteryven
Copy link
Contributor

You can run just ready in your local to fix the Clippy error.

@woai3c
Copy link
Contributor Author

woai3c commented Apr 17, 2024

@mysteryven Are there any other tasks I can work on? If not, I will check if there are any additional rules I can add for ESLint or Prettier.

@mysteryven
Copy link
Contributor

mysteryven commented Apr 17, 2024

I will check if there are any additional rules I can add for ESLint or Prettier.

Try a new ESLint rule or any issue you like!


I think we need to solve the performance issue, it decreases a bit large, I'll help to check the reason when I free.(usually at night :<), do you have discord account, you can add me mysteryven, let's discuss it!

@woai3c
Copy link
Contributor Author

woai3c commented Apr 17, 2024

I will check if there are any additional rules I can add for ESLint or Prettier.

Try a new ESLint rule or any issue you like!

I think we need to solve the performance issue, it decreases a bit large, I'll help to check the reason when I free.(usually at night :<), do you have discord account, you can add me mysteryven, let's discuss it!

Ok, I have sent a friend request.

@mysteryven
Copy link
Contributor

I changed the category to style, let's merge it first.

@mysteryven mysteryven changed the title feat(linter): add max_len rule feat(linter): eslint/max-len Apr 19, 2024
@mysteryven mysteryven merged commit 9b4e87a into oxc-project:main Apr 19, 2024
32 of 33 checks passed
Brooooooklyn referenced this pull request in toeverything/AFFiNE Apr 22, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [oxlint](https://oxc-project.github.io) ([source](https://togithub.com/oxc-project/oxc/tree/HEAD/npm/oxlint)) | [`0.2.17` -> `0.3.0`](https://renovatebot.com/diffs/npm/oxlint/0.2.17/0.3.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/oxlint/0.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/oxlint/0.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/oxlint/0.2.17/0.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/oxlint/0.2.17/0.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>oxc-project/oxc (oxlint)</summary>

### [`v0.3.0`](https://togithub.com/oxc-project/oxc/releases/tag/oxlint_v0.3.0): oxlint v0.3.0

[Compare Source](https://togithub.com/oxc-project/oxc/compare/04f5fc018650d9a5dc6a4b1b40dea941fa07781e...b29aabd6f1a2e9e8cfa7db25c371ab40f79d02a5)

#### What's Changed

##### Potential breaking change

-   `-D all` no longer enables nursery rules, use `-D all -D nursery` instead

##### Features

-   support eslint globals by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/3038](https://togithub.com/oxc-project/oxc/pull/3038)
-   change no-empty-static-block to correctness
-   implement `--format checkstyle` by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/3044](https://togithub.com/oxc-project/oxc/pull/3044)
-   implement `--format unix` by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/3039](https://togithub.com/oxc-project/oxc/pull/3039)
-   implement fixer for `typescript-eslint/consistent-type-definitions` by [@&#8203;todor-a](https://togithub.com/todor-a) in [https://github.com/oxc-project/oxc/pull/3045](https://togithub.com/oxc-project/oxc/pull/3045)

##### Fixes

-   fix crashing with `unwrap` in import/no-cycle by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/3035](https://togithub.com/oxc-project/oxc/pull/3035)

**Full Changelog**: oxc-project/oxc@oxlint_v0.2.18...oxlint_v0.3.0

### [`v0.2.18`](https://togithub.com/oxc-project/oxc/releases/tag/oxlint_v0.2.18): oxlint v0.2.18

[Compare Source](https://togithub.com/oxc-project/oxc/compare/df11d10a2220e9aa7a33d9ab39ed662c2ba6fdb5...04f5fc018650d9a5dc6a4b1b40dea941fa07781e)

#### What's Changed

##### New Rules

-   correctness: eslint-plugin-unicorn no await in promise methods by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/2963](https://togithub.com/oxc-project/oxc/pull/2963)
-   correctness: eslint-plugin-unicorn no single promise in promise methods by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/2962](https://togithub.com/oxc-project/oxc/pull/2962)

##### Features

-   Add --jsdoc-plugin flag by [@&#8203;leaysgur](https://togithub.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/2935](https://togithub.com/oxc-project/oxc/pull/2935)
-   Implement plugin-jsdoc/check-property-names by [@&#8203;leaysgur](https://togithub.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/2989](https://togithub.com/oxc-project/oxc/pull/2989)
-   eslint/max-len by [@&#8203;woai3c](https://togithub.com/woai3c) in [https://github.com/oxc-project/oxc/pull/2874](https://togithub.com/oxc-project/oxc/pull/2874)
-   remove import/no-unresolved by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/3023](https://togithub.com/oxc-project/oxc/pull/3023)
-   support `oxlint-disable` alongside `eslint-disable` by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/3024](https://togithub.com/oxc-project/oxc/pull/3024)
-   jsdoc: Implement require-property rule by [@&#8203;leaysgur](https://togithub.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/3011](https://togithub.com/oxc-project/oxc/pull/3011)
-   jsdoc: Implement require-property-(type|name|description) rules by [@&#8203;leaysgur](https://togithub.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/3013](https://togithub.com/oxc-project/oxc/pull/3013)

#### New Contributors

-   [@&#8203;branchseer](https://togithub.com/branchseer) made their first contribution in [https://github.com/oxc-project/oxc/pull/2943](https://togithub.com/oxc-project/oxc/pull/2943)
-   [@&#8203;woai3c](https://togithub.com/woai3c) made their first contribution in [https://github.com/oxc-project/oxc/pull/2874](https://togithub.com/oxc-project/oxc/pull/2874)

**Full Changelog**: oxc-project/oxc@oxlint_v0.2.17...oxlint_v0.2.18

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/toeverything/AFFiNE).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjMxMy4xIiwidGFyZ2V0QnJhbmNoIjoiY2FuYXJ5IiwibGFiZWxzIjpbImRlcGVuZGVuY2llcyJdfQ==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants