From b0dbcdcdb69c3a66503baa71c808ff868da1b9e5 Mon Sep 17 00:00:00 2001
From: Raymond Gasper <raymond.j.gasper@gmail.com>
Date: Tue, 29 Sep 2020 21:53:06 -0400
Subject: [PATCH] metrics show: allow falsey values (#4642)

* metrics show: allow falsey values

Fixes #4630

* metrics show: bug in logging to console. Fixes #4630

* repro: fix incorrect usage of _show_metrics

* fix formatting/styling

* metrics: add a simple func test

Co-authored-by: Ruslan Kuprieiev <ruslan@iterative.ai>
---
 dvc/command/metrics.py             | 17 +++++++++++------
 dvc/command/repro.py               |  5 +++--
 dvc/repo/metrics/show.py           |  4 ++--
 tests/func/metrics/test_show.py    |  7 +++++++
 tests/unit/command/test_metrics.py | 21 ++++++++++++++++++++-
 5 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/dvc/command/metrics.py b/dvc/command/metrics.py
index 55e4fcb57..fcbb5843a 100644
--- a/dvc/command/metrics.py
+++ b/dvc/command/metrics.py
@@ -11,7 +11,7 @@ logger = logging.getLogger(__name__)
 DEFAULT_PRECISION = 5
 
 
-def show_metrics(
+def _show_metrics(
     metrics, all_branches=False, all_tags=False, all_commits=False
 ):
     from dvc.utils.diff import format_dict
@@ -21,22 +21,25 @@ def show_metrics(
     # specified as `targets` in `repo.metrics.show` didn't contain any metrics.
     missing = metrics.pop(None, None)
 
+    lines = []
     for branch, val in metrics.items():
         if all_branches or all_tags or all_commits:
-            logger.info(f"{branch}:")
+            lines.append(f"{branch}:")
 
         for fname, metric in val.items():
             if not isinstance(metric, dict):
-                logger.info("\t{}: {}".format(fname, str(metric)))
+                lines.append("\t{}: {}".format(fname, str(metric)))
                 continue
 
-            logger.info(f"\t{fname}:")
+            lines.append(f"\t{fname}:")
             for key, value in flatten(format_dict(metric)).items():
-                logger.info(f"\t\t{key}: {value}")
+                lines.append(f"\t\t{key}: {value}")
 
     if missing:
         raise BadMetricError(missing)
 
+    return "\n".join(lines)
+
 
 class CmdMetricsBase(CmdBase):
     UNINITIALIZED = True
@@ -58,12 +61,14 @@ class CmdMetricsShow(CmdMetricsBase):
 
                 logger.info(json.dumps(metrics))
             else:
-                show_metrics(
+                table = _show_metrics(
                     metrics,
                     self.args.all_branches,
                     self.args.all_tags,
                     self.args.all_commits,
                 )
+                if table:
+                    logger.info(table)
         except DvcException:
             logger.exception("failed to show metrics")
             return 1
diff --git a/dvc/command/repro.py b/dvc/command/repro.py
index 18962feb4..cc5c69e17 100644
--- a/dvc/command/repro.py
+++ b/dvc/command/repro.py
@@ -4,7 +4,7 @@ import os
 
 from dvc.command import completion
 from dvc.command.base import CmdBase, append_doc_link
-from dvc.command.metrics import show_metrics
+from dvc.command.metrics import _show_metrics
 from dvc.command.status import CmdDataStatus
 from dvc.dvcfile import PIPELINE_FILE
 from dvc.exceptions import DvcException
@@ -51,7 +51,8 @@ class CmdRepro(CmdBase):
 
                 if self.args.metrics:
                     metrics = self.repo.metrics.show()
-                    show_metrics(metrics)
+                    logger.info(_show_metrics(metrics))
+
             except DvcException:
                 logger.exception("")
                 ret = 1
diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py
index d0e7b07ae..d189bd659 100644
--- a/dvc/repo/metrics/show.py
+++ b/dvc/repo/metrics/show.py
@@ -48,7 +48,7 @@ def _extract_metrics(metrics, path, rev):
     ret = {}
     for key, val in metrics.items():
         m = _extract_metrics(val, path, rev)
-        if m:
+        if m not in (None, {}):
             ret[key] = m
         else:
             logger.debug(
@@ -80,7 +80,7 @@ def _read_metrics(repo, metrics, rev):
             continue
 
         val = _extract_metrics(val, metric, rev)
-        if val:
+        if val not in (None, {}):
             res[str(metric)] = val
 
     return res
diff --git a/tests/func/metrics/test_show.py b/tests/func/metrics/test_show.py
index ae9a42bd4..96d3f9d01 100644
--- a/tests/func/metrics/test_show.py
+++ b/tests/func/metrics/test_show.py
@@ -160,3 +160,10 @@ def test_non_metric_and_recurisve_show(tmp_dir, dvc, run_copy_metrics):
             "metrics_t.yaml": {"foo": 1.1},
         }
     }
+
+
+def test_show_falsey(tmp_dir, dvc):
+    tmp_dir.gen("metrics.json", '{"foo": 0, "bar": 0.0, "baz": {}}')
+    assert dvc.metrics.show(targets=["metrics.json"]) == {
+        "": {"metrics.json": {"foo": 0, "bar": 0.0}}
+    }
diff --git a/tests/unit/command/test_metrics.py b/tests/unit/command/test_metrics.py
index 89429eb79..1e8650556 100644
--- a/tests/unit/command/test_metrics.py
+++ b/tests/unit/command/test_metrics.py
@@ -1,7 +1,12 @@
 import textwrap
 
 from dvc.cli import parse_args
-from dvc.command.metrics import CmdMetricsDiff, CmdMetricsShow, _show_diff
+from dvc.command.metrics import (
+    CmdMetricsDiff,
+    CmdMetricsShow,
+    _show_diff,
+    _show_metrics,
+)
 
 
 def test_metrics_diff(dvc, mocker):
@@ -233,3 +238,17 @@ def test_metrics_diff_with_old():
         metrics.yaml  a.d.e     3      4      1
         metrics.yaml  x.b       5      6      1"""
     )
+
+
+def test_metrics_show_with_valid_falsey_values():
+    assert _show_metrics(
+        {"branch_1": {"metrics.json": {"a": 0, "b": {"ad": 0.0, "bc": 0.0}}}},
+        all_branches=True,
+    ) == textwrap.dedent(
+        """\
+        branch_1:
+        \tmetrics.json:
+        \t\ta: 0
+        \t\tb.ad: 0.0
+        \t\tb.bc: 0.0"""
+    )