Repository navigation
Conversation
Signed-off-by: Andras Mitzki <andras.mitzki@balabit.com>
|
Maybe the warning message is not the best, not concrete. |
|
I was also thinking about the exiting if there is an unexpected top level key. |
| ) | ||
|
|
||
|
|
||
| _TOP_LEVEL_KEYS = ('repo', 'rev', 'hooks') |
There was a problem hiding this comment.
ideally this should be derived from CONFIG_REPO_SCHEMA in pre_commit.clientlib -- it probably also makes sense to implement this check as a cfgv checker in that module
also these aren't top level keys, just repository-level keys
|
|
||
|
|
||
| def _check_extra_top_level_keys(repo_config): | ||
| extra_top_level_keys = set(dict(repo_config).keys()) - set(_TOP_LEVEL_KEYS) |
There was a problem hiding this comment.
there's 4 unnecessary copies happening here:
dict(repo_config).keys()(in python 2)set(dict...)set(_TOP_LEVEL_KEYS)
The module constant should be frozenset((...)), you can use set(repo_config) directly
| if extra_top_level_keys: | ||
| logger.warning( | ||
| 'Unexpected top level keys present: {}, only supported' | ||
| 'top level keys: {}'.format( |
There was a problem hiding this comment.
this produces a not-great error message that contains python literals, use ', '.join(sorted(...)) when displaying lists
| extra_top_level_keys = set(dict(repo_config).keys()) - set(_TOP_LEVEL_KEYS) | ||
| if extra_top_level_keys: | ||
| logger.warning( | ||
| 'Unexpected top level keys present: {}, only supported' |
There was a problem hiding this comment.
let's use consistent messaging with the warning on hook-dict extra keys (it's in this module under create)
|
Thanks, for the review notes. I will fix them. |
can't error without warning first, don't want to break 5 years of behaviour |
|
I like to add this check into |
|
yeah you'd want something like that but |
|
@mitzkia with cfgv 1.6 you should be able to finish this 👍 |
|
via #1028 |
This is a fix for: #965
Of course I will also add unit-tests here.
Signed-off-by: Andras Mitzki andras.mitzki@balabit.com