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

Add schemas for options (and remove for files which are using settings) #234

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Feb 17, 2020

If you want me to keep the schemas for settings, I can instead comment them out, but AFAICT, they are only usable with options, not settings.

Note that I've had to comment out one test case in each of the two valid-*-description rules for this PR as the new schema causes some RegExp literals to fail on a couple properties (since JSON Schema can only specify JSON types and does not support indicating a RegExp except as a string).

@lo1tuma
Copy link
Owner

lo1tuma commented Feb 17, 2020

If you want me to keep the schemas for settings, I can instead comment them out, but AFAICT, they are only usable with options, not settings.

👍 good catch! Seems like I did a bad job during the code review, since the actual feature was first implemented as rule options and then refactored, due to my review, to settings. Thanks for removing that.

Note that I've had to comment out one test case in each of the two valid-*-description rules

Actually I haven’t noticed that until now. I don’t think this was intentional. It seems like this slipped in while a different feature was added in #206 and it is also not documented. So I would be ok with removing those test-cases and also the whole functionality.

@brettz9
Copy link
Contributor Author

brettz9 commented Feb 17, 2020

I've amended to remove the regex literal test cases (there were two others but the schema wasn't complaining about them) as well as the detection of RegExp instances. And I've rebased.

Copy link
Owner

@lo1tuma lo1tuma left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants