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

Proposal: Cleaner JSON Schemas #3625

Closed
nzakas opened this Issue Sep 1, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@nzakas
Copy link
Member

nzakas commented Sep 1, 2015

I've been thinking about this for a bit, and I kind of hate what we have to do to support optional configuration options, like this:

"enum": [0, 1, 2]

IMHO, needing to put the error codes into the rules, explicitly, is a barrier to having a clean and loosely-coupled API.

Therefore, I'd like to propose that we no longer require rules to include enum: [1, 2, 3] in any case and that the rule schema should, instead, only refer to the values provided in context.options.

How would this work?

Basically, the validator will always just shift() off the first option (the severity) and manually validate that number. We already have a special case to provide a better message (https://github.com/eslint/eslint/blob/master/lib/config-validator.js#L83), but really, we don't need JSON schema to determine if a value is 0, 1, or 2. We can just do that ourselves.

So we remove the severity from the array and then apply the JSON schema.

The end result is that no schema ever needs to worry about severity codes under any circumstances.

Thoughts?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 1, 2015

It looks like linked example from no-restricted-syntax is done differently then most of our schemas. In most places we don't do schema as an object, we do it as an array, and this line of code takes care of the error levels for us.
So I think currently no rule schema should ever need to worry about severity code, unless the person who created that schema really wants to do that, for some reason.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Sep 1, 2015

That was just one example. Here's another:

"enum": [0, 1, 2]

This was recommended for whenever rules can have varying numbers of options. I think this needs to be addressed so it doesn't continue to be used.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 2, 2015

Oh, I see now. That's done so that you could do

    "uniqueItems": true,
    "minItems": 1

Sorry, I missed that part. In that case I agree.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Sep 2, 2015

👍

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Sep 2, 2015

👍 This design is much cleaner. I see no reason why it wouldn't work.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Sep 2, 2015

Cool, I'll add this to the 2.0.0 roadmap.

@nzakas nzakas added accepted and removed evaluating labels Sep 2, 2015

@nzakas nzakas added this to the v2.0.0 milestone Sep 2, 2015

@nzakas nzakas referenced this issue Sep 2, 2015

Closed

Roadmap: 2.0.0 #3561

11 of 11 tasks complete
@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Sep 3, 2015

If I understand this proposal correctly then is it really a breaking change (asking from a consumer perspective)?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 3, 2015

It would be a breaking change only for plugin creators, consumers will not see anything at all.

P.S. I didn't check but I would bet that not a lot of plugins are using schemas yet anyways, especially object notation for schema, so even in the case of plugin creators, it might only break a few plugins if any.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Sep 3, 2015

Got it. Thanks @ilyavolodin

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 1, 2015

I'll take a stab at this, I think it should be fairly easy.

@nzakas nzakas closed this in 8d028c0 Dec 4, 2015

nzakas added a commit that referenced this issue Dec 4, 2015

Merge pull request #4596 from eslint/issue3625
Breaking: Simplify rule schemas (fixes #3625)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.