Linter: create rule to remove other validation keywords when enum is present based on type #1899
Conversation
…based on type Signed-off-by: karan-palan <[email protected]>
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
… string Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
|
@jviotti any comments? |
Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
Signed-off-by: karan-palan <[email protected]>
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
| this->blacklist.clear(); | ||
| for (const auto &entry : schema.as_object()) { | ||
| const auto metadata = walker(entry.first, vocabularies); | ||
| if (metadata.type == sourcemeta::core::SchemaKeywordType::Other || |
There was a problem hiding this comment.
Do we really need this if? What example would be problematic if we removed this?
| "minimum": 10, | ||
| "minLength": 2, | ||
| "minItems": 1, | ||
| "minProperties": 1, |
There was a problem hiding this comment.
Your tests, mainly for older versions, are not considering the fact that many of these keywords do not exist in such older versions. For example, minProperties is only present in Draft 4.
The test works because in Draft 0, minProperties is not considered to be a keyword, therefore it applies to "any" type.
However, to make the tests cleaner, can you double check that you are not using any seemingly valid keyword on a Draft version that doesn't actually define it?
To test the above case, we can also have specific new tests that have a non-sense x-foo-bar keyword or something like that, and ensure it doesn't get removed
Signed-off-by: karan-palan <[email protected]>
Signed-off-by: karan-palan <[email protected]>
Signed-off-by: karan-palan <[email protected]>
| @@ -0,0 +1,75 @@ | |||
| class EnumValidationKeywordsDefault final : public SchemaTransformRule { | |||
There was a problem hiding this comment.
The name of the class doesn't match the file name?
| } | ||
| } | ||
|
|
||
| return !this->blacklist.empty(); |
There was a problem hiding this comment.
A bit of a minor thing, but can you print the offending keywords like we do for x-?
There was a problem hiding this comment.
Tests look good, but can you add some tests that exercise enum alongside other non-validation keywords, like:
- Annotations (i.e.
title) - Anchors (i.e.
$anchor) - Maybe
$id,$comment, and other ones?
The rule removes: