8000 feat(rome_js_analyze): useNamingConvention by Conaclos · Pull Request #4534 · rome/tools · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): useNamingConvention - #4534

Merged
Conaclos merged 9 commits into
rome:mainfrom
Conaclos:useNamingConvention
Jun 18, 2023
Merged

feat(rome_js_analyze): useNamingConvention#4534
Conaclos merged 9 commits into
rome:mainfrom
Conaclos:useNamingConvention

Conversation

@Conaclos
@Conaclos Conaclos commented May 26, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Summary

Implementation of useNamingConvention.

Test Plan

I added a large test suite.

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
netlify Bot commented May 26, 2023 •
edited
Loading
Copy link
Copy Markdown

✅ Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 2679661
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/648f4553b24fdc0008c21fab

@github-actions github-actions Bot added A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading labels May 26, 2023
@Conaclos
Conaclos marked this pull request as draft May 26, 2023 15:45
@Conaclos
Conaclos marked this pull request as ready for review June 6, 2023 14:41
@Conaclos
Conaclos commented Jun 6, 2023
Copy link
Copy Markdown
Contributor Author

Hi!

There are still some code to polish, in particular the documentation.
I could like to have a first review.

I am wondering: is there a way to test a rule config?

@Conaclos
Conaclos requested a review from ematipico June 6, 2023 14:43
@ematipico
ematipico commented Jun 6, 2023 •
edited by Conaclos
Loading
Copy link
Copy Markdown
Contributor

Hi!

There are still some code to polish, in particular the documentation.
I could like to have a first review.

I am wondering: is there a way to test a rule config?

I have this PR open that will change how we parse and test rules with options. We could merge it, if you think that helps .

To test a rule with configuration, create an *.options.json file, where * is the name of the file you want to testtest. The react hooks rules have some examples

@Conaclos
Conaclos commented Jun 6, 2023
Copy link
Copy Markdown
Contributor Author

I have this PR open that will change how we parse and test rules with options. We could merge it, if you think that helps .

You mean #4541?

This could be nice!

@ematipico
8000
ematipico commented Jun 10, 2023
Copy link
Copy Markdown
Contributor

@Conaclos I merged the PR that changes how options are deserialized.

Now you can better document the options.

You will have to regenerate the schema and bindings.

@Conaclos
Conaclos commented Jun 11, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@ematipico

I updated the code to validate/hydrate the rule configuration.

Something looking wrong for me is the union of all rule options in the schema. Is this normal?

There are also some redundancy between the rule option visitor and the visitor of PossibleOptions. This looks also wrong to me because this breaks composability.

By the way, my current implementation is correctly validated. However, the configuration does not take effect s seen in unit test validEnumMemberConstantCase.ts.snap. Could you help me to figure out why?

Also, the json schema informs the user to set both strictCase and enumMeberCase. Implementing the default trait should not be enough to make these two fields optional?

@ematipico
ematipico commented Jun 12, 2023
Copy link
Copy Markdown
Contributor

@ematipico

I updated the code to validate/hydrate the rule configuration.

Something looking wrong for me is the union of all rule options in the schema. Is this normal?

There are also some redundancy between the rule option visitor and the visitor of PossibleOptions. This looks also wrong to me because this breaks composability.

I need clarification on what you think it's wrong. Would you mind pointing out the code or explaining in more detail?

By the way, my current implementation is correctly validated. However, the configuration does not take effect s seen in unit test validEnumMemberConstantCase.ts.snap. Could you help me to figure out why?

Have you tried to debug it? You can use the quick_test test function inside rome_js_analyze/src/lib.rs. Add the code snippet that you want and the rule to test. And you can pass the configuration that you need. Doing so, you can use the debugger and understand why the options are not correctly deserialized.

Also, the json schema informs the user to set both strictCase and enumMeberCase. Implementing the default trait should not be enough to make these two fields optional?

Implementing Default is not enough. You need to:

  1. implement Option for those options that you think can be optional
  2. add #[serde(skip_serializing_if = "Option::is_none")] to those options that are optional

ematipico
ematipico reviewed Jun 12, 2023
View reviewed changes
Comment thread crates/rome_js_syntax/src/member_name_ext.rs Resolved
Comment thread crates/rome_js_syntax/src/member_name_ext.rs Resolved
Comment thread crates/rome_js_syntax/src/member_name_ext.rs Resolved
Comment thread crates/rome_js_syntax/src/modifier_ext.rs Resolved
Comment thread crates/rome_js_syntax/src/modifier_ext.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
@Conaclos
Conaclos commented Jun 14, 2023
Copy link
Copy Markdown
Contributor Author

I applied most of your suggestions.

For now, I am not completely satisfied with the printed diagnostics. I will rework this part.

@Conaclos
Conaclos commented Jun 15, 2023
Copy link
Copy Markdown
Contributor Author

I am ready for the final review and merge :)

@Conaclos
Conaclos requested a review from ematipico June 16, 2023 11:38
ematipico
ematipico reviewed Jun 16, 2023
View reviewed changes
@ematipico ematipico left a comment •
edited
Loading
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's one thing that concerns me, which is the code action. I failed to find a test case where we have the same incorrect name used multiple times, and how the code action behaves (same thing for the diagnostic).

For example:

const PascalVariable = 2;
console.log(PascalVariable);

how's the diagnostic and how's the code action?

About the documentation: you often the "should" instead to use the imperative. The "should" implies a conditional, which implies something that "is not sure" or "up to interpretation". Since we are writing about a rule, I think we should use more "decisive" tenses.

Comment thread crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/malformedOptions.js.snap Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
Comment thread crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs Resolved
@ematipico
ematipico commented Jun 16, 2023
Copy link
Copy Markdown
Contributor

Also, could you upgrade your insta version in your machine? Doing so will remove all the assertion lines inside the snapshots.

@ematipico
ematipico commented Jun 16, 2023
Copy link
Copy Markdown
Contributor

The Test node.js API CI is failing. I think you will have to update the snapshots

@Conaclos
Conaclos commented Jun 16, 2023
Copy link
Copy Markdown
Contributor Author

The Test node.js API CI is failing. I think you will have to update the snapshots

Have you some docs about this?

@ematipico
ematipico commented Jun 17, 2023
Copy link
Copy Markdown
Contributor

The Test node.js API CI is failing. I think you will have to update the snapshots

Have you some docs about this?

Not at the moment, since the JS API are very experimental.

This the home of the project: https://github.com/rome/tools/tree/main/npm%2Fjs-api

You would need to build the WASM packages. The package.json has the scripts you need. You'd need to install wasm-pack globally: https://rustwasm.github.io/wasm-pack/installer/

Once the packages are built, you should be able to run the tests and be able to update the snapshots.

unvalley
unvalley reviewed Jun 18, 2023
View reviewed changes
Comment thread CHANGELOG.md Resolved
@Conaclos
Conaclos commented Jun 18, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@ematipico

I am stuck.

I installed wasm-pack from my distribution repository (installing with cargo install results in segmentation faults when running wasm-pack).

I run npm install on every package in ./npm/.

I got the following error when running npm run build:bundler-dev / npm run build:node-dev / npm run build:web-dev:

❯ npm run build:wasm-bundler-dev

> @rometools/js-api@0.3.0 build:wasm-bundler-dev
> wasm-pack build --out-dir ../../npm/wasm-bundler --target bundler --dev --scope rometools ../../crates/rome_wasm

[INFO]: 🎯  Checking for the Wasm target...
[INFO]: 🌀  Compiling to Wasm...
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
[INFO]: ⬇️  Installing wasm-bindgen...
error: failed getting Wasm module for '~/project/rome_tools/target/wasm32-unknown-unknown/debug/rome_wasm.wasm'

Caused by:
    0: failed to parse input as wasm
    1: failed to parse code section
    2: locals exceed maximum (at offset 18689800)
Error: Running the wasm-bindgen CLI
Caused by: Running the wasm-bindgen CLI
Caused by: failed to execute `wasm-bindgen`: exited with exit status: 1
  full command: "~/.cache/.wasm-pack/wasm-bindgen-5f26acfc988649a3/wasm-bindgen" "~/project/rome_tools/target/wasm32-unknown-unknown/debug/rome_wasm.wasm" "--out-dir" "../../crates/rome_wasm/../../npm/wasm-bundler" "--typescript" "--target" "bundler" "--debug"

By the way running these commands without -dev suffix succeed. However, I got thes ame issue with vitest failing (rome.shutdown call fails because rome variable is undefined)

Then I run the following command: cargo build --target=wasm32-unknown-unknown, and I obtain the following errors:

error[E0432]: unresolved import `crate::sys::IoSourceState`
  --> ~/.local/share/cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-0.8.6/src/io_source.rs:12:5
   |
12 | use crate::sys::IoSourceState;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ no `IoSourceState` in `sys`

[and a lot more...]

@ematipico
ematipico commented Jun 18, 2023
Copy link
Copy Markdown
Contributor

That's OK. Not a blocker. We can disable the job for now and move on; it's not a priority that API.

@ematipico
ematipico commented Jun 18, 2023
Copy link
Copy Markdown
Contributor

FYI the locals limit was hit already few months ago. I did digging: wasm-bindgen/wasm-bindgen#3451

Will try to fix it in another PR when I have time. If someone wants to help, that's very welcome

@Conaclos
Conaclos commented Jun 18, 2023
Copy link
Copy Markdown
Contributor Author

I finally identified the issue :)

@Conaclos
Conaclos merged commit ae62733 into rome:main Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@ematipico ematipico ematipico left review comments
@unvalley unvalley unvalley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Conaclos @ematipico @unvalley
0