Skip to content

Commit

Permalink
pw_presubmit: Add keep_sorted check
Browse files Browse the repository at this point in the history
Add a keep_sorted presubmit check inspired by the corresponding internal
tool: http://go/keep-sorted. Also add 'pw keep-sorted' which sorts these
blocks automatically.

Bug: b/250875082
Change-Id: Id8d74c942c93176df8b2881339fbda2cc797346e
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/112211
Commit-Queue: Auto-Submit <[email protected]>
Reviewed-by: Wyatt Hepler <[email protected]>
Pigweed-Auto-Submit: Rob Mohr <[email protected]>
  • Loading branch information
mohrr authored and CQ Bot Account committed Oct 8, 2022
1 parent 8a47742 commit 56d8316
Show file tree
Hide file tree
Showing 8 changed files with 446 additions and 14 deletions.
3 changes: 2 additions & 1 deletion PW_PLUGINS
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# virtual environment. The function must have no required arguments and should
# return an int to use as the exit code.

# Pigweed's presubmit check script
# keep-sorted: start
format pw_presubmit.format_code _pigweed_upstream_main
heap-viewer pw_allocator.heap_viewer main
ide pw_ide.__main__ main
Expand All @@ -19,3 +19,4 @@ presubmit pw_presubmit.pigweed_presubmit main
requires pw_cli.requires main
rpc pw_hdlc.rpc_console main
console pw_console.__main__ main
# keep-sorted: end
2 changes: 2 additions & 0 deletions pw_cli/py/pw_cli/pw_command_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def _register_builtin_plugins(registry: plugins.Registry) -> None:
registry.register_by_name('python-packages',
'pw_env_setup.python_packages', 'main')
registry.register_by_name('format', 'pw_presubmit.format_code', 'main')
registry.register_by_name('keep-sorted', 'pw_presubmit.keep_sorted',
'main')
registry.register_by_name('logdemo', 'pw_cli.log', 'main')
registry.register_by_name('module', 'pw_module.__main__', 'main')
registry.register_by_name('test', 'pw_unit_test.test_runner', 'main')
Expand Down
20 changes: 20 additions & 0 deletions pw_presubmit/docs.rst
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,26 @@ all use language-specific formatters like clang-format or yapf.

These will suggest fixes using ``pw format --fix``.

Sorted Blocks
^^^^^^^^^^^^^
Blocks of code can be required to be kept in sorted order using comments like
the following:

.. code-block::
# keep-sorted: start
bar
baz
foo
# keep-sorted: end
This can be included by adding ``pw_presubmit.keep_sorted.keep_sorted`` to a
presubmit program.

These will suggest fixes using ``pw keep-sorted --fix``.

Future versions may support multiline list items and case-insensitive sorting.

#pragma once
^^^^^^^^^^^^
There's a ``pragma_once`` check that confirms the first non-comment line of
Expand Down
2 changes: 2 additions & 0 deletions pw_presubmit/py/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pw_python_package("py") {
"pw_presubmit/git_repo.py",
"pw_presubmit/inclusive_language.py",
"pw_presubmit/install_hook.py",
"pw_presubmit/keep_sorted.py",
"pw_presubmit/npm_presubmit.py",
"pw_presubmit/pigweed_presubmit.py",
"pw_presubmit/presubmit.py",
Expand All @@ -41,6 +42,7 @@ pw_python_package("py") {
]
tests = [
"git_repo_test.py",
"keep_sorted_test.py",
"presubmit_test.py",
"tools_test.py",
]
Expand Down
104 changes: 104 additions & 0 deletions pw_presubmit/py/keep_sorted_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
#!/usr/bin/env python3
# Copyright 2022 The Pigweed Authors
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not
# use this file except in compliance with the License. You may obtain a copy of
# the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations under
# the License.
"""Tests for keep_sorted."""

from pathlib import Path
import tempfile
import unittest
from unittest.mock import MagicMock

from pw_presubmit import keep_sorted

# Only include these literals here so keep_sorted doesn't try to reorder later
# test lines.
START = keep_sorted.START
END = keep_sorted.END

# pylint: disable=attribute-defined-outside-init


class TestKeepSorted(unittest.TestCase):
"""Test KeepSorted class"""
def _run(self, contents: str) -> None:
self.ctx = MagicMock()
self.ctx.fail = MagicMock()

with tempfile.TemporaryDirectory() as tempdir:
path = Path(tempdir) / 'foo'

with path.open('w') as outs:
outs.write(contents)

# pylint: disable=protected-access
self.sorter = keep_sorted._FileSorter(self.ctx, path)

# pylint: enable=protected-access

self.sorter.sort()

# Truncate the file so it's obvious whether write() changed
# anything.
with path.open('w') as outs:
outs.write('')

self.sorter.write(path)
with path.open() as ins:
self.contents = ins.read()

def test_missing_end(self) -> None:
with self.assertRaises(keep_sorted.KeepSortedParsingError):
self._run(f'{START}\n')

def test_missing_start(self) -> None:
with self.assertRaises(keep_sorted.KeepSortedParsingError):
self._run(f'{END}: end\n')

def test_repeated_start(self) -> None:
with self.assertRaises(keep_sorted.KeepSortedParsingError):
self._run(f'{START}\n{START}\n')

def test_already_sorted(self) -> None:
self._run(f'{START}\n1\n2\n3\n4\n{END}\n')
self.ctx.fail.assert_not_called()
self.assertEqual(self.contents, '')

def test_not_sorted(self) -> None:
self._run(f'{START}\n4\n3\n2\n1\n{END}\n')
self.ctx.fail.assert_called()
self.assertEqual(self.contents, f'{START}\n1\n2\n3\n4\n{END}\n')

def test_prefix_sorted(self) -> None:
self._run(f'foo\nbar\n{START}\n1\n2\n{END}\n')
self.ctx.fail.assert_not_called()
self.assertEqual(self.contents, '')

def test_prefix_not_sorted(self) -> None:
self._run(f'foo\nbar\n{START}\n2\n1\n{END}\n')
self.ctx.fail.assert_called()
self.assertEqual(self.contents, f'foo\nbar\n{START}\n1\n2\n{END}\n')

def test_suffix_sorted(self) -> None:
self._run(f'{START}\n1\n2\n{END}\nfoo\nbar\n')
self.ctx.fail.assert_not_called()
self.assertEqual(self.contents, '')

def test_suffix_not_sorted(self) -> None:
self._run(f'{START}\n2\n1\n{END}\nfoo\nbar\n')
self.ctx.fail.assert_called()
self.assertEqual(self.contents, f'{START}\n1\n2\n{END}\nfoo\nbar\n')


if __name__ == '__main__':
unittest.main()
Loading

0 comments on commit 56d8316

Please sign in to comment.