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

Simplify linting output in workflow #1640

Merged
merged 1 commit into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions .github/workflows/linting.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,17 @@ jobs:
- name: Check documentation
id: documentation-fabric
run: |
python3 tools/check_documentation.py --show-diffs modules fast blueprints
python3 tools/check_documentation.py --show-diffs --no-show-summary modules fast blueprints

- name: Check documentation links
id: documentation-links-fabric
run: |
python3 tools/check_links.py .
python3 tools/check_links.py --no-show-summary .

- name: Check name length (fast)
id: name-length-fast
run: |
python3 tools/check_names.py --prefix-length=10 fast/stages
python3 tools/check_names.py --prefix-length=10 --failed-only fast/stages

- name: Check python formatting
id: yapf
Expand All @@ -76,4 +76,4 @@ jobs:
- name: Check blueprint metadata
id: metadata
run: |
python tools/validate_metadata.py -v blueprints
python tools/validate_metadata.py -v --failed-only blueprints
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,8 @@ Options:
--help Show this message and exit.
```

As a convenience, we provide a script that runs the same set of checks as our GitHub workflow. Before submitting a PR, run `tools/lint.sh` and fix any errors. You can use the tools described above to find out more about the failures.

The test workflow runs test suites in parallel. Refer to the next section for more details on running and writing tests.

## Using and writing tests
Expand Down
34 changes: 18 additions & 16 deletions tools/check_documentation.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def _check_dir(dir_name, exclude_files=None, files=False, show_extra=False):

diff = None
readme = readme_path.read_text()
mod_name = str(readme_path.relative_to(dir_path).parent)
readme_rel = str(readme_path.relative_to(BASEDIR))
current_doc = tfdoc.get_tfref_parts(readme)
current_toc = tfdoc.get_toc_parts(readme)
if current_doc or current_toc:
Expand All @@ -88,63 +88,63 @@ def _check_dir(dir_name, exclude_files=None, files=False, show_extra=False):

if current_doc and new_doc.content != current_doc['doc']:
state = State.FAIL_STALE_README
header = f'----- {mod_name} diff -----\n'
header = f'----- {readme_rel} diff -----\n'
ndiff = difflib.ndiff(current_doc['doc'].splitlines(keepends=True),
new_doc.content.splitlines(keepends=True))
diff = ''.join([header] + [x for x in ndiff if x[0] != ' '])

elif current_toc and new_toc != current_toc['toc']:
state = State.FAIL_STALE_TOC
header = f'----- {mod_name} diff -----\n'
header = f'----- {readme_rel} diff -----\n'
ndiff = difflib.ndiff(current_toc['toc'].splitlines(keepends=True),
new_toc.splitlines(keepends=True))
diff = ''.join([header] + [x for x in ndiff if x[0] != ' '])

elif empty := [v.name for v in newvars if not v.description]:
state = state.FAIL_VARIABLE_DESCRIPTION
diff = "\n".join([
f'----- {mod_name} variables missing description -----',
f'----- {readme_rel} variables missing description -----',
', '.join(empty),
])

elif empty := [o.name for o in newouts if not o.description]:
state = state.FAIL_VARIABLE_DESCRIPTION
diff = "\n".join([
f'----- {mod_name} outputs missing description -----',
f'----- {readme_rel} outputs missing description -----',
', '.join(empty),
])

elif variables != sorted(variables):
state = state.FAIL_UNSORTED_VARS
diff = "\n".join([
f'----- {mod_name} variables -----',
f'----- {readme_rel} variables -----',
f'variables should be in this order: ',
', '.join(sorted(variables)),
])

elif outputs != sorted(outputs):
state = state.FAIL_UNSORTED_OUTPUTS
diff = "\n".join([
f'----- {mod_name} outputs -----',
f'----- {readme_rel} outputs -----',
f'outputs should be in this order: ',
', '.join(sorted(outputs)),
])

elif nc := [v.name for v in newvars if not v.description.endswith('.')]:
state = state.FAIL_VARIABLE_PERIOD
diff = "\n".join([
f'----- {mod_name} variable descriptions missing ending period -----',
f'----- {readme_rel} variable descriptions missing ending period -----',
', '.join(nc),
])

elif nc := [o.name for o in newouts if not o.description.endswith('.')]:
state = state.FAIL_OUTPUT_PERIOD
diff = "\n".join([
f'----- {mod_name} output descriptions missing ending period -----',
f'----- {readme_rel} output descriptions missing ending period -----',
', '.join(nc),
])

yield mod_name, state, diff
yield readme_rel, state, diff


@click.command()
Expand All @@ -153,18 +153,19 @@ def _check_dir(dir_name, exclude_files=None, files=False, show_extra=False):
@click.option('--files/--no-files', default=False)
@click.option('--show-diffs/--no-show-diffs', default=False)
@click.option('--show-extra/--no-show-extra', default=False)
@click.option('--show-summary/--no-show-summary', default=True)
def main(dirs, exclude_file=None, files=False, show_diffs=False,
show_extra=False):
show_extra=False, show_summary=True):
'Cycle through modules and ensure READMEs are up-to-date.'
print(f'files: {files}, extra: {show_extra}, diffs: {show_diffs}\n')
# print(f'files: {files}, extra: {show_extra}, diffs: {show_diffs}\n')
errors = []
for dir_name in dirs:
print(f'----- {dir_name} -----')
result = _check_dir(dir_name, exclude_file, files, show_extra)
for mod_name, state, diff in result:
for readme_path, state, diff in result:
if state.failed:
errors.append((mod_name, diff))
print(f'[{state.label}] {mod_name}')
errors.append((readme_path, diff))
if show_summary:
print(f'[{state.label}] {readme_path}')

if errors:
if show_diffs:
Expand All @@ -173,6 +174,7 @@ def main(dirs, exclude_file=None, files=False, show_diffs=False,
else:
print('Errored modules:')
print('\n'.join([e[0] for e in errors]))

raise SystemExit('Errors found.')


Expand Down
14 changes: 9 additions & 5 deletions tools/check_links.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env python3

# Copyright 2022 Google LLC
# Copyright 2023 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -77,20 +77,24 @@ def check_docs(dir_name, external=False):
@click.argument('dirs', type=str, nargs=-1)
@click.option('-e', '--external', is_flag=True, default=False,
help='Whether to test external links.')
def main(dirs, external):
@click.option('--show-summary/--no-show-summary', default=True)
def main(dirs, external, show_summary=True):
'Checks links in Markdown files contained in dirs.'
errors = []
for dir_name in dirs:
print(f'----- {dir_name} -----')
if show_summary:
print(f'----- {dir_name} -----')
for doc in check_docs(dir_name, external):
state = '✓' if all(l.valid for l in doc.links) else '✗'
print(f'[{state}] {doc.relpath} ({len(doc.links)})')
if show_summary:
print(f'[{state}] {doc.relpath} ({len(doc.links)})')
if state == '✗':
error = [f'{dir_name}/{doc.relpath}']
for l in doc.links:
if not l.valid:
error.append(f' - {l.dest}')
print(f' {l.dest}')
if show_summary:
print(f' {l.dest}')
errors.append('\n'.join(error))
if errors:
raise SystemExit('Errors found:\n{}'.format('\n'.join(errors)))
Expand Down
20 changes: 12 additions & 8 deletions tools/check_names.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/usr/bin/env python3
# Copyright 2022 Google LLC
# Copyright 2023 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -72,7 +72,8 @@ def get_names(dir_name):
@click.command()
@click.argument('dirs', type=str, nargs=-1)
@click.option('--prefix-length', default=7, type=int)
def main(dirs, prefix_length=None):
@click.option('--failed-only', is_flag=True, default=False)
def main(dirs, prefix_length=None, failed_only=False):
'Parse names in dirs.'
import json
logging.basicConfig(level=logging.INFO)
Expand All @@ -87,18 +88,21 @@ def main(dirs, prefix_length=None):
errors = []
for name in names:
name_length = name.length + prefix_length
do_print = True
if name_length >= MOD_LIMITS[name.source]:
flag = "✗"
errors += [f"{name.source}:{name.name}:{name_length}"]
else:
flag = "✓"
do_print = not failed_only

print(f"[{flag}] {name.source.ljust(source_just)} "
f"{name.name.ljust(name_just)} "
f"{name.value.ljust(value_just)} "
f"({name_length})")
if errors:
raise ValueError(errors)
if do_print:
print(f"[{flag}] {name.source.ljust(source_just)} "
f"{name.name.ljust(name_just)} "
f"{name.value.ljust(value_just)} "
f"({name_length})")

return 0 if not errors else 1


if __name__ == '__main__':
Expand Down
6 changes: 4 additions & 2 deletions tools/validate_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ def _validate(path: Path, validator) -> ValidationResult:
@click.argument('dirs', type=click.Path(exists=True, file_okay=False), nargs=-1)
@click.option('-v', '--verbose', is_flag=True, default=False,
help='Print additional validation details.')
def main(dirs: list[str], verbose: bool) -> int:
@click.option('--failed-only', is_flag=True, default=False)
def main(dirs: list[str], verbose: bool, failed_only=False) -> int:
instances = set()
for dir_name in dirs:
instances |= set(Path(dir_name).glob("**/metadata.yaml"))
Expand All @@ -72,7 +73,8 @@ def main(dirs: list[str], verbose: bool) -> int:
for instance in instances:
result = _validate(instance, validator)
if result.state == State.OK:
print(f'[✓] {instance}')
if not failed_only:
print(f'[✓] {instance}')
else:
print(f'[✗] {instance}')
failed_files[instance] = result.errors
Expand Down