feat: exclude common inconsistent property names #230
Conversation
christiancompton
force-pushed
the
inconsistent_property
branch
from
7c59cc7
to
2e33ea9
Compare
src/.defaultsForValidator.js
Outdated
| @@ -65,7 +65,7 @@ const defaults = { | |||
| 'no_property_description': 'warning', | |||
| 'description_mentions_json': 'warning', | |||
| 'array_of_arrays': 'warning', | |||
| 'inconsistent_property_type': 'warning', | |||
| 'inconsistent_property_type': ['warning', 'code', 'default', 'type', 'value'], | |||
There was a problem hiding this comment.
Would it make more sense this way?
| 'inconsistent_property_type': ['warning', 'code', 'default', 'type', 'value'], | |
| 'inconsistent_property_type': ['warning', ['code', 'default', 'type', 'value']], |
src/.defaultsForValidator.js
Outdated
| @@ -65,7 +65,7 @@ const defaults = { | |||
| 'no_property_description': 'warning', | |||
| 'description_mentions_json': 'warning', | |||
| 'array_of_arrays': 'warning', | |||
| 'inconsistent_property_type': 'warning', | |||
| 'inconsistent_property_type': ['warning', 'code', 'default', 'type', 'value'], | |||
There was a problem hiding this comment.
I would make a suggestion for a small tweak here. I think the list of keys to exclude should be their own list. Right now, it looks to me like these values are separate arguments with potentially different purposes. Since they are all meant to be grouped as a list though, I think it makes more sense to have them be one argument in list form, like:
['warning', ['code', 'default', 'type', 'value']]
README.md
Outdated
| @@ -483,7 +483,7 @@ The default values for each rule are described below. | |||
| | no_property_description | warning | | |||
| | description_mentions_json | warning | | |||
| | array_of_arrays | warning | | |||
| | inconsistent_property_type | warning | | |||
| | inconsistent_property_type | warning, code, default, type, value] | | |||
There was a problem hiding this comment.
For rules with config options like warning, lower_snake_case and case_convention in the name, it's fairly obvious what the configuration represents.
In this new case, it may not be as obvious. I think we should document what the list means. Something like
| | inconsistent_property_type | warning, code, default, type, value] | | |
| | inconsistent_property_type | warning, [code, default, type, value] | (List of property names excluded for this check) |
I don't know if that would screw up the table formatting though. Maybe there's another place we could document it.
There was a problem hiding this comment.
I did need to modify this slightly to get the table to render properly
| printed: false | ||
| }; | ||
| if (configOption && !configOption.includes(key)) { | ||
| // add property if the name is not generic |
There was a problem hiding this comment.
This is really picky 😅, but users may exclude non-generic properties
| // add property if the name is not generic | |
| // add property if the name is not excluded |
christiancompton
force-pushed
the
inconsistent_property
branch
3 times, most recently
from
1a70f0d
to
312e331
Compare
christiancompton
force-pushed
the
inconsistent_property
branch
from
312e331
to
fe653ac
Compare
# [0.34.0](v0.33.2...v0.34.0) (2021-01-26) ### Features * exclude common inconsistent property names ([#230](#230)) ([d1b9909](d1b9909))
|
🎉 This PR is included in version 0.34.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #181 .