Skip to content

Check unsupported TOP_LEVEL_KEYS #980

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.

Sign up for GitHub

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

Closed
mitzkia wants to merge 1 commit into from
Closed

Check unsupported TOP_LEVEL_KEYS #980

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pre_commit/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ def _prefix(language_name, deps):


def _cloned_repository_hooks(repo_config, store, root_config):
_check_extra_top_level_keys(repo_config)
repo, rev = repo_config['repo'], repo_config['rev']
manifest_path = os.path.join(store.clone(repo, rev), C.MANIFEST_FILE)
by_id = {hook['id']: hook for hook in load_manifest(manifest_path)}
Expand Down Expand Up @@ -190,6 +191,21 @@ def _cloned_repository_hooks(repo_config, store, root_config):
)


_TOP_LEVEL_KEYS = ('repo', 'rev', 'hooks')

@asottile asottile Mar 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

@asottile asottile Mar 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there's 4 unnecessary copies happening here:

  1. dict(repo_config)
  2. .keys() (in python 2)
  3. set(dict...)
  4. 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'

@asottile asottile Mar 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's use consistent messaging with the warning on hook-dict extra keys (it's in this module under create)

'top level keys: {}'.format(

@asottile asottile Mar 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this produces a not-great error message that contains python literals, use ', '.join(sorted(...)) when displaying lists

extra_top_level_keys,
_TOP_LEVEL_KEYS,
),
)


def _repository_hooks(repo_config, store, root_config):
if repo_config['repo'] in {LOCAL, META}:
return _non_cloned_repository_hooks(repo_config, store, root_config)
Expand Down