feat(rome_js_analyze): useNamingConvention #4534

Conaclos · 2023-05-26T15:45:29Z

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 · 2023-05-26T15:45:37Z

✅ 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

Conaclos · 2023-06-06T14:43:32Z

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?

ematipico · 2023-06-06T14:48:26Z

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 · 2023-06-06T18:28:34Z

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 · 2023-06-10T08:59:31Z

@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 · 2023-06-11T18:33:36Z

@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 · 2023-06-12T09:39:48Z

@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:

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

crates/rome_js_syntax/src/member_name_ext.rs

crates/rome_js_syntax/src/modifier_ext.rs

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs

Conaclos · 2023-06-14T14:22:43Z

I applied most of your suggestions.

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

Conaclos · 2023-06-15T16:27:00Z

I am ready for the final review and merge :)

ematipico

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.

crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/malformedOptions.js.snap

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_naming_convention.rs

ematipico · 2023-06-16T12:04:51Z

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

ematipico · 2023-06-16T13:00:49Z

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

Conaclos · 2023-06-16T15:34:41Z

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

Have you some docs about this?

ematipico · 2023-06-17T17:50:38Z

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.

CHANGELOG.md

Conaclos · 2023-06-18T16:33:35Z

@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 · 2023-06-18T16:54:01Z

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

ematipico · 2023-06-18T17:09:14Z

FYI the locals limit was hit already few months ago. I did digging: rustwasm/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 · 2023-06-18T18:14:02Z

I finally identified the issue :)

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 marked this pull request as draft May 26, 2023 15:45

Conaclos marked this pull request as ready for review June 6, 2023 14:41

Conaclos requested a review from ematipico June 6, 2023 14:43

ematipico reviewed Jun 12, 2023

View reviewed changes

Conaclos requested a review from ematipico June 16, 2023 11:38

ematipico reviewed Jun 16, 2023

View reviewed changes

unvalley reviewed Jun 18, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Conaclos added 3 commits June 18, 2023 19:18

feat(rome_js_analyze): useNamingConvention

a5bbb61

refactor: use a regular enum instead of a bitflag

b172791

refactor: fix naming convention of Rome js code

1be9155

Conaclos added 6 commits June 18, 2023 19:18

fix: configuration deserialization

487bb98

refactor: take review into account

3b2f1cd

refactor: improve docs and implemntation

fa436de

refactor: code and diagnostic polishing

16c558b

refactor: second review

ad2a08b

fix: try to fix ci fail

2679661

Conaclos merged commit ae62733 into rome:main Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyze): useNamingConvention #4534

feat(rome_js_analyze): useNamingConvention #4534

Conaclos commented May 26, 2023 •

edited

Loading

netlify bot commented May 26, 2023 •

edited

Loading

Conaclos commented Jun 6, 2023

ematipico commented Jun 6, 2023 •

edited by Conaclos

Loading

Conaclos commented Jun 6, 2023

ematipico commented Jun 10, 2023

Conaclos commented Jun 11, 2023 •

edited

Loading

ematipico commented Jun 12, 2023

Conaclos commented Jun 14, 2023

Conaclos commented Jun 15, 2023

ematipico left a comment •

edited

Loading

ematipico commented Jun 16, 2023

ematipico commented Jun 16, 2023

Conaclos commented Jun 16, 2023

ematipico commented Jun 17, 2023

Conaclos commented Jun 18, 2023 •

edited

Loading

ematipico commented Jun 18, 2023

ematipico commented Jun 18, 2023

Conaclos commented Jun 18, 2023

feat(rome_js_analyze): useNamingConvention #4534

feat(rome_js_analyze): useNamingConvention #4534

Conversation

Conaclos commented May 26, 2023 • edited Loading

Summary

Test Plan

Changelog

Documentation

netlify bot commented May 26, 2023 • edited Loading

✅ Deploy Preview for docs-rometools canceled.

Conaclos commented Jun 6, 2023

ematipico commented Jun 6, 2023 • edited by Conaclos Loading

Conaclos commented Jun 6, 2023

ematipico commented Jun 10, 2023

Conaclos commented Jun 11, 2023 • edited Loading

ematipico commented Jun 12, 2023

Conaclos commented Jun 14, 2023

Conaclos commented Jun 15, 2023

ematipico left a comment • edited Loading

Choose a reason for hiding this comment

ematipico commented Jun 16, 2023

ematipico commented Jun 16, 2023

Conaclos commented Jun 16, 2023

ematipico commented Jun 17, 2023

Conaclos commented Jun 18, 2023 • edited Loading

ematipico commented Jun 18, 2023

ematipico commented Jun 18, 2023

Conaclos commented Jun 18, 2023

Conaclos commented May 26, 2023 •

edited

Loading

netlify bot commented May 26, 2023 •

edited

Loading

ematipico commented Jun 6, 2023 •

edited by Conaclos

Loading

Conaclos commented Jun 11, 2023 •

edited

Loading

ematipico left a comment •

edited

Loading

Conaclos commented Jun 18, 2023 •

edited

Loading