diff options
author | Zuul <zuul@review.opendev.org> | 2021-02-09 18:35:31 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2021-02-09 18:35:31 +0000 |
commit | a5bdcc6e789adb0182df0721909d7b657f9a50b5 (patch) | |
tree | d973449be47710c7b13376735bf8dd47bbb93d1c | |
parent | fca202dfb2e22d88f95e0bcce848ec4839f745cf (diff) | |
parent | 4f45f9a30e657265c5b5ac119af3aaa0e5ec7184 (diff) | |
download | cliff-a5bdcc6e789adb0182df0721909d7b657f9a50b5.tar.gz |
Merge "Handle null values when sorting"
-rw-r--r-- | cliff/lister.py | 25 | ||||
-rw-r--r-- | cliff/tests/test_lister.py | 54 | ||||
-rw-r--r-- | releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml | 8 |
3 files changed, 80 insertions, 7 deletions
diff --git a/cliff/lister.py b/cliff/lister.py index cfba233..dde0f2d 100644 --- a/cliff/lister.py +++ b/cliff/lister.py @@ -13,7 +13,7 @@ """Application base class for providing a list of data as output. """ import abc -import operator +import logging from . import display @@ -22,6 +22,8 @@ class Lister(display.DisplayCommandBase, metaclass=abc.ABCMeta): """Command base class for providing a list of data as output. """ + log = logging.getLogger(__name__) + @property def formatter_namespace(self): return 'cliff.formatter.list' @@ -63,8 +65,25 @@ class Lister(display.DisplayCommandBase, metaclass=abc.ABCMeta): if parsed_args.sort_columns and self.need_sort_by_cliff: indexes = [column_names.index(c) for c in parsed_args.sort_columns if c in column_names] - if indexes: - data = sorted(data, key=operator.itemgetter(*indexes)) + for index in indexes[::-1]: + try: + # We need to handle unset values (i.e. None) so we sort on + # multiple conditions: the first comparing the results of + # an 'is None' type check and the second comparing the + # actual value. The second condition will only be checked + # if the first returns True, which only happens if the + # returns from the 'is None' check on the two values are + # the same, i.e. both None or both not-None + data = sorted( + data, key=lambda k: (k[index] is None, k[index]), + ) + except TypeError: + # Simply log and then ignore this; sorting is best effort + self.log.warning( + "Could not sort on field '%s'; unsortable types", + parsed_args.sort_columns[index], + ) + (columns_to_include, selector) = self._generate_columns_and_selector( parsed_args, column_names) if selector: diff --git a/cliff/tests/test_lister.py b/cliff/tests/test_lister.py index 8603004..7fc7222 100644 --- a/cliff/tests/test_lister.py +++ b/cliff/tests/test_lister.py @@ -32,16 +32,15 @@ class FauxFormatter(object): class ExerciseLister(lister.Lister): + data = [('a', 'A'), ('b', 'B'), ('c', 'A')] + def _load_formatter_plugins(self): return { 'test': FauxFormatter(), } def take_action(self, parsed_args): - return ( - parsed_args.columns, - [('a', 'A'), ('b', 'B'), ('c', 'A')], - ) + return (parsed_args.columns, self.data) class ExerciseListerCustomSort(ExerciseLister): @@ -49,6 +48,16 @@ class ExerciseListerCustomSort(ExerciseLister): need_sort_by_cliff = False +class ExerciseListerNullValues(ExerciseLister): + + data = ExerciseLister.data + [(None, None)] + + +class ExerciseListerDifferentTypes(ExerciseLister): + + data = ExerciseLister.data + [(1, 0)] + + class TestLister(base.TestBase): def test_formatter_args(self): @@ -111,6 +120,43 @@ class TestLister(base.TestBase): data = list(args[1]) self.assertEqual([['a', 'A'], ['b', 'B'], ['c', 'A']], data) + def test_sort_by_column_with_null(self): + test_lister = ExerciseListerNullValues(mock.Mock(), []) + parsed_args = mock.Mock() + parsed_args.columns = ('Col1', 'Col2') + parsed_args.formatter = 'test' + parsed_args.sort_columns = ['Col2', 'Col1'] + + test_lister.run(parsed_args) + + f = test_lister._formatter_plugins['test'] + args = f.args[0] + data = list(args[1]) + self.assertEqual( + [['a', 'A'], ['c', 'A'], ['b', 'B'], [None, None]], data) + + def test_sort_by_column_with_different_types(self): + test_lister = ExerciseListerDifferentTypes(mock.Mock(), []) + parsed_args = mock.Mock() + parsed_args.columns = ('Col1', 'Col2') + parsed_args.formatter = 'test' + parsed_args.sort_columns = ['Col2', 'Col1'] + + with mock.patch.object(lister.Lister, 'log') as mock_log: + test_lister.run(parsed_args) + + f = test_lister._formatter_plugins['test'] + args = f.args[0] + data = list(args[1]) + # The output should be unchanged + self.assertEqual( + [['a', 'A'], ['b', 'B'], ['c', 'A'], [1, 0]], data) + # but we should have logged a warning + mock_log.warning.assert_has_calls([ + mock.call("Could not sort on field '%s'; unsortable types", col) + for col in parsed_args.sort_columns + ]) + def test_sort_by_non_displayed_column(self): test_lister = ExerciseLister(mock.Mock(), []) parsed_args = mock.Mock() diff --git a/releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml b/releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml new file mode 100644 index 0000000..a7368c1 --- /dev/null +++ b/releasenotes/notes/handle-none-values-when-sorting-de40e36c66ad95ca.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Sorting output using the ``--sort-column`` option will now handle ``None`` + values. This was supported implicitly in Python 2 but was broken in the + move to Python 3. In addition, requests to sort a column containing + non-comparable types will now be ignored. Previously, these request would + result in a ``TypeError``. |