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

Validate rule schemas #182

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Validate rule schemas #182

merged 1 commit into from
Jan 30, 2020

Conversation

fvictorio
Copy link
Contributor

The schemas of the rules configurations now only represent the configuration itself (meaning, the second element of the array when setting a rule). Then, base-checker validates that the schema is correct. If it's not, only a warning is emitted, execution is not interrupted.

Lot of files to review here, but the important part is in base-checker.

@fvictorio fvictorio requested a review from fernandomg January 30, 2020 16:31
@fvictorio fvictorio changed the base branch from master to 3.0 January 30, 2020 16:31
@coveralls
Copy link

coveralls commented Jan 30, 2020

Coverage Status

Coverage decreased (-0.3%) to 95.99% when pulling c2c1fb3 on validate-rule-schemas into e07b908 on 3.0.

@fvictorio fvictorio force-pushed the validate-rule-schemas branch from 7b8c9ae to c2c1fb3 Compare January 30, 2020 16:34
@fernandomg
Copy link
Contributor

fernandomg commented Jan 30, 2020

@fvictorio, it would be nice to have the ajv validated scheme to inform which are those allowed values.

node solhint.js Foo.sol
[solhint] Warning: invalid configuration for rule 'max-line-length':
  should be equal to one of the allowed values
[solhint] Warning: rule 'avoid-sha3' level is 'eror'; should be one of "error", "warn" or "off"

the config file:

{
  "rules": {
    "avoid-sha3": "eror",
    "max-line-length": ["not-a-valid-value", 10],
    "avoid-throw": "error"
  }
}

what do you think?

@fvictorio
Copy link
Contributor Author

In this particular case, we can do it "by hand": checking for the first element of the array the same way it's being done when the configuration is just a string (i.e., the level).

But this won't translate to rules that use the oneOf validator of ajv. But we could use better-ajv-errors for this.

@fernandomg
Copy link
Contributor

I was thinking about using the params prop. https://ajv.js.org/#error-parameters

image

Copy link
Contributor

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

This works great.

Now I get the issue with oneOf.

@fernandomg fernandomg merged commit 265d95f into 3.0 Jan 30, 2020
@fernandomg fernandomg deleted the validate-rule-schemas branch January 30, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants