+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

refactor(rome_service): move globals, refactor rule configuration #2907

Merged
merged 9 commits into from
Jul 22, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jul 20, 2022

Summary

This PR does four things

globals field

This field has been moved inside the JavaScript language, it is now like this

{
	"root": true,
  	"javascript": {
        "globals": ["$"],
    	"formatter": {
      		"quoteStyle": "double"
    	},
  	}
}

rules settings

Now the rules can be configured in two different way. The old one and a new one, in case we will need to configure some rules:

{
    "noLabelVar": {
        "level": "warn",
        "options": "test_option"
    },
    "useTemplate": {
        "level": "error",
        "options": [5]
    }
}

options is defined as a Value for now, which means that anything can be assigned to it. I don't have a clear idea how this field will be de-serialized/configured and documented inside our code generation, but at least this data structured gives us an edge where we don't need to apply breaking changes, because options is now a wildcard.

rome init command

Now the command will turn off the linter by default, as we at our last meeting. I also added more text to the command. This text is exactly the same text that the Rome classic used to show. I didn't change anything, other than some link.

Configuration validation

In my last PR I didn't add the attribute deny_unknown_fields to all the structs. I was under the impression that the attribute worked for all the struct once defined in the main one, but nope! It should work now

Test Plan

Updated the existing cases to match the structure of the new configuration file.

Here's how the rome init is shown. Unfortunately the current markup macro is a bit difficult to tame when it comes to add tabbing and newlines.

Screenshot 2022-07-20 at 13 55 19

@ematipico ematipico requested a review from leops as a code owner July 20, 2022 14:57
@ematipico ematipico requested a review from a team July 20, 2022 14:57
@ematipico ematipico temporarily deployed to aws July 20, 2022 14:57 Inactive
@github-actions
Copy link

github-actions bot commented Jul 20, 2022

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 20, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9dd22b2
Status: ✅  Deploy successful!
Preview URL: https://10ce985a.tools-8rn.pages.dev
Branch Preview URL: https://refactor-linter-configuratio.tools-8rn.pages.dev

View logs

@ematipico ematipico temporarily deployed to aws July 20, 2022 16:45 Inactive
@ematipico ematipico temporarily deployed to aws July 20, 2022 16:48 Inactive
@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5946 5946 0
Passed 395 395 0
Failed 5551 5551 0
Panics 0 0 0
Coverage 6.64% 6.64% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12393 12393 0
Failed 3864 3864 0
Panics 0 0 0
Coverage 76.23% 76.23% 0.00%

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

globals

{
	"root": true,
  	"javascript": {
    	"formatter": {
      		"quoteStyle": "double"
    	},
    	"linter": {
        	"globals": ["$"]
    	}
  	}
}
  • How do you intend to represent globals for other languages like HTML (global web component names), or CSS (global styles)?
  • I would move globals out of linters because knowing the globals is also something that is useful in the context of modification where globals should never be mangled. Tough, closure uses explicit extern files that also provide type annotations for better results.

Init command

What's the migration strategy if Rome's default change between two versions? Do users have to explicitly upgrade their configuration or should new defaults be applied automatically?

Let's say we release the linter and enable it by default. Do now all Rome users have to enable the linter manually to make use of it?

@ematipico ematipico temporarily deployed to aws July 21, 2022 10:36 Inactive
@ematipico
Copy link
Contributor Author

@leops @MichaReiser

The configuration of rome init command is now enabled by default. I added a TODO for adding the recommendation settings.

@MichaReiser

I would move globals out of linters

Done. It makes total sense.

How do you intend to represent globals for other languages like HTML (global web component names), or CSS (global styles)?

I am not sure I fully understand the question. But for CSS I would see something like globals: ["--external-variable"] and then our analyzer infrastructure will pick it up when running the rules.

What's the migration strategy if Rome's default change between two versions? Do users have to explicitly upgrade their configuration or should new defaults be applied automatically?

Let's say we release the linter and enable it by default. Do now all Rome users have to enable the linter manually to make use of it?

Migrations for configuration files was in plan for Rome classic. There are two ideas on the table, which can leave together. Having a version field, and when that is lower than the current one, we can run transformations to upgrade it to the new defaults.

The other idea is to map transformations of certain sections of the configuration from one version to another. For example, let's say we decided to rename foo from bar, and this change occurs at version 10.5.0, then we create a migration for that field to rename itself. This requires semantic versioning and parsing/transforming of the JSON language.

@ematipico ematipico temporarily deployed to aws July 21, 2022 10:56 Inactive
@ematipico ematipico force-pushed the refactor/linter-configuration branch from c87bd17 to cd670dd Compare July 21, 2022 11:17
@ematipico ematipico temporarily deployed to aws July 21, 2022 11:17 Inactive
@ematipico ematipico temporarily deployed to aws July 21, 2022 11:58 Inactive
@ematipico ematipico merged commit 7bed9cd into main Jul 22, 2022
@ematipico ematipico deleted the refactor/linter-configuration branch July 22, 2022 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载