Stop gap measure to avoid always ignoring ignore config (#542)

* Stop gap measure to avoid always ignoring ignore config

* Add tests for CLI ignore config vs flags

* Improve differe reset fixture use

* Use differ reset more in tests now

nbdime/args.py

@@ -162,6 +162,8 @@ def process_exclusive_ignorables(ns, arg_names, default=True):
 
     It checks that all specified options are either all positive or all negative.
     It then returns a namespace with the parsed options.
+
+    Returns whether any values were specified or not.
     """
     # `toggle` tracks whether:
     #  - True: One or more positive options were defined
@@ -184,6 +186,7 @@ def process_exclusive_ignorables(ns, arg_names, default=True):
     for name in arg_names:
         if getattr(ns, name) is None:
             setattr(ns, name, default)
+    return toggle is not None
 
 
 def add_generic_args(parser):
@@ -384,9 +387,12 @@ def add_git_diff_driver_args(diff_parser):
 
 
 def process_diff_flags(args):
-    process_exclusive_ignorables(args, diff_ignorables)
-    set_notebook_diff_targets(args.sources, args.outputs,
-                              args.attachments, args.metadata, args.details)
+    any_flags_given = process_exclusive_ignorables(args, diff_ignorables)
+    if any_flags_given:
+        # Note: This will blow away any options set via config (for these fields)
+        set_notebook_diff_targets(
+            args.sources, args.outputs, args.attachments, args.metadata,
+            args.details)
 
 
 def resolve_diff_args(args):

nbdime/nbdiffapp.py

@@ -90,10 +90,11 @@ def write(self, text):
     return 0
 
 
-def _build_arg_parser():
+def _build_arg_parser(prog=None):
     """Creates an argument parser for the nbdiff command."""
     parser = ConfigBackedParser(
         description=_description,
+        prog=prog,
         )
     add_generic_args(parser)
     add_diff_args(parser)

nbdime/tests/conftest.py

@@ -21,7 +21,7 @@
 
 from .utils import call, have_git, have_hg, wait_up, TEST_TOKEN
 
-from nbdime.diffing.notebooks import set_notebook_diff_targets
+from nbdime.diffing.notebooks import reset_notebook_differ
 
 try:
     # Python >= 3.3
@@ -417,12 +417,12 @@ def unique_port():
 
 
 @fixture()
-def reset_diff_targets():
+def reset_notebook_diff():
     try:
         yield
     finally:
-        # Reset diff targets (global variable)
-        set_notebook_diff_targets()
+        # Reset differ (global variable)
+        reset_notebook_differ()
 
 @fixture()
 def popen_with_terminator(request):

nbdime/tests/test_args.py

@@ -71,7 +71,7 @@ def test_config_parser(entrypoint_config):
     assert arguments.log_level == 'ERROR'
 
 
-def test_ignore_config_simple(entrypoint_ignore_config, tmpdir):
+def test_ignore_config_simple(entrypoint_ignore_config, tmpdir, reset_notebook_diff):
     tmpdir.join('nbdime_config.json').write_text(
         text_type(json.dumps({
             'IgnorableConfig1': {
@@ -91,20 +91,14 @@ def mock_ignore_keys(inner, keys):
     try:
         with tmpdir.as_cwd():
             parser.parse_args([])
-    except:
-        nbdime.diffing.notebooks.reset_notebook_differ()
-        raise
     finally:
         nbdime.diffing.notebooks.diff_ignore_keys = old_keys
 
-    try:
-        assert notebook_differs['/cells/*/metadata'] == (diff, ['collapsed', 'autoscroll'])
-    finally:
-        nbdime.diffing.notebooks.reset_notebook_differ()
+    assert notebook_differs['/cells/*/metadata'] == (diff, ['collapsed', 'autoscroll'])
 
 
 
-def test_ignore_config_merge(entrypoint_ignore_config, tmpdir):
+def test_ignore_config_merge(entrypoint_ignore_config, tmpdir, reset_notebook_diff):
     tmpdir.join('nbdime_config.json').write_text(
         text_type(json.dumps({
             'IgnorableConfig1': {
@@ -130,21 +124,15 @@ def mock_ignore_keys(inner, keys):
     try:
         with tmpdir.as_cwd():
             parser.parse_args([])
-    except:
-        nbdime.diffing.notebooks.reset_notebook_differ()
-        raise
     finally:
         nbdime.diffing.notebooks.diff_ignore_keys = old_keys
 
-    try:
-        assert notebook_differs['/metadata'] == (diff, ['foo'])
-        # Lists are not merged:
-        assert notebook_differs['/cells/*/metadata'] == (diff, ['tags'])
-    finally:
-        nbdime.diffing.notebooks.reset_notebook_differ()
+    assert notebook_differs['/metadata'] == (diff, ['foo'])
+    # Lists are not merged:
+    assert notebook_differs['/cells/*/metadata'] == (diff, ['tags'])
 
 
-def test_config_inherit(entrypoint_ignore_config, tmpdir):
+def test_config_inherit(entrypoint_ignore_config, tmpdir, reset_notebook_diff):
     tmpdir.join('nbdime_config.json').write_text(
         text_type(json.dumps({
             'IgnorableConfig1': {
@@ -158,7 +146,4 @@ def test_config_inherit(entrypoint_ignore_config, tmpdir):
     with tmpdir.as_cwd():
         parsed = parser.parse_args([])
 
-    try:
-        assert parsed.metadata is False
-    finally:
-        nbdime.diffing.notebooks.reset_notebook_differ()
+    assert parsed.metadata is False

nbdime/tests/test_cli_apps.py

@@ -7,6 +7,8 @@
 import json
 import logging
 import os
+from pprint import pprint
+from six import text_type
 from subprocess import CalledProcessError, check_call
 import sys
 import time
@@ -24,6 +26,7 @@
 
 import nbdime
 from nbdime.args import process_exclusive_ignorables
+from nbdime.diffing.notebooks import notebook_differs
 from nbdime.nbshowapp import main_show
 from nbdime.nbdiffapp import main_diff
 from nbdime.nbmergeapp import main_merge
@@ -121,7 +124,7 @@ def test_nbdiff_app_unicode_safe(filespath):
     check_call([sys.executable, '-m', 'nbdime.nbdiffapp', afn, bfn], env=env)
 
 
-def test_nbdiff_app_only_source(filespath, tmpdir, reset_diff_targets):
+def test_nbdiff_app_only_source(filespath, tmpdir, reset_notebook_diff):
     afn = os.path.join(filespath, "multilevel-test-base.ipynb")
     bfn = os.path.join(filespath, "multilevel-test-local.ipynb")
     dfn = os.path.join(tmpdir.dirname, "diff_output.json")
@@ -137,7 +140,7 @@ def test_nbdiff_app_only_source(filespath, tmpdir, reset_diff_targets):
         diff = diff[0]['diff']
 
 
-def test_nbdiff_app_ignore_source(filespath, tmpdir, reset_diff_targets):
+def test_nbdiff_app_ignore_source(filespath, tmpdir, reset_notebook_diff):
     afn = os.path.join(filespath, "multilevel-test-base.ipynb")
     bfn = os.path.join(filespath, "multilevel-test-local.ipynb")
     dfn = os.path.join(tmpdir.dirname, "diff_output.json")
@@ -153,7 +156,7 @@ def test_nbdiff_app_ignore_source(filespath, tmpdir, reset_diff_targets):
         diff = diff[0]['diff']
 
 
-def test_nbdiff_app_only_details(filespath, tmpdir, reset_diff_targets):
+def test_nbdiff_app_only_details(filespath, tmpdir, reset_notebook_diff):
     afn = os.path.join(filespath, "single_cell_nb.ipynb")
     bfn = os.path.join(filespath, "single_cell_nb--changed_source_output_ec.ipynb")
     dfn = os.path.join(tmpdir.dirname, "diff_output.json")
@@ -174,7 +177,7 @@ def test_nbdiff_app_only_details(filespath, tmpdir, reset_diff_targets):
     assert diff[0]['value'] == 2
 
 
-def test_nbdiff_app_ignore_details(filespath, tmpdir, reset_diff_targets):
+def test_nbdiff_app_ignore_details(filespath, tmpdir, reset_notebook_diff):
     afn = os.path.join(filespath, "single_cell_nb.ipynb")
     bfn = os.path.join(filespath, "single_cell_nb--changed_source_output_ec.ipynb")
     dfn = os.path.join(tmpdir.dirname, "diff_output.json")
@@ -197,6 +200,73 @@ def test_nbdiff_app_ignore_details(filespath, tmpdir, reset_diff_targets):
     assert diff[1]['key'] == 'source'
 
 
+def test_nbdiff_app_config_ignores(filespath, tmpdir, reset_notebook_diff):
+    tmpdir.join('nbdime_config.json').write_text(
+        text_type(json.dumps({
+            'Diff': {
+                'Ignore': {
+                    '/cells/*/metadata': ['nbdime-dummy-field']
+                }
+            },
+        })),
+        encoding='utf-8'
+    )
+
+    afn = os.path.join(filespath, "single_cell_nb.ipynb")
+    bfn = os.path.join(filespath, "single_cell_nb--changed_metadata.ipynb")
+    dfn = os.path.join(tmpdir.dirname, "diff_output.json")
+    with tmpdir.as_cwd():
+        args = nbdiffapp._build_arg_parser('nbdiff').parse_args([afn, bfn, '--out', dfn])
+    assert 0 == main_diff(args)
+
+    pprint(notebook_differs)
+
+    with io.open(dfn) as df:
+        diff = json.load(df)
+    pprint(diff)
+    for key in ('metadata', 'language_info'):
+        assert len(diff) == 1
+        assert diff[0]['key'] == key
+        assert diff[0]['op'] == 'patch'
+        diff = diff[0]['diff']
+
+    assert len(diff) == 1
+    assert diff[0]['key'] == 'version'
+    assert diff[0]['op'] == 'patch'
+
+
+def test_nbdiff_app_flags_override_config_ignores(filespath, tmpdir, reset_notebook_diff):
+    # This excercises current behavior, but should ideally (?) be different
+
+    tmpdir.join('nbdime_config.json').write_text(
+        text_type(json.dumps({
+            'Diff': {
+                'Ignore': {
+                    '/cells/*/metadata': ['nbdime-dummy-field']
+                }
+            },
+        })),
+        encoding='utf-8'
+    )
+
+    afn = os.path.join(filespath, "single_cell_nb.ipynb")
+    bfn = os.path.join(filespath, "single_cell_nb--changed_metadata.ipynb")
+    dfn = os.path.join(tmpdir.dirname, "diff_output.json")
+    with tmpdir.as_cwd():
+        args = nbdiffapp._build_arg_parser('nbdiff').parse_args([afn, bfn, '--out', dfn, '-S'])
+    assert 0 == main_diff(args)
+
+    pprint(notebook_differs)
+
+    with io.open(dfn) as df:
+        diff = json.load(df)
+    pprint(diff)
+
+    assert len(diff) == 2
+    assert diff[0]['key'] == 'cells'
+    assert diff[1]['key'] == 'metadata'
+
+
 def test_nbdiff_app_color_words(filespath):
     # Simply check that the --color-words argument is accepted, not behavior
     afn = os.path.join(filespath, "multilevel-test-base.ipynb")

nbdime/tests/test_git_diffdriver.py

@@ -185,7 +185,7 @@
 
 """
 
-def test_git_diff_driver(filespath, capsys, needs_git):
+def test_git_diff_driver(filespath, capsys, needs_git, reset_notebook_diff):
     # Simulate a call from `git diff` to check basic driver functionality
 
     fn1 = pjoin(filespath, 'foo--1.ipynb')
@@ -207,7 +207,7 @@ def test_git_diff_driver(filespath, capsys, needs_git):
         assert cap_out == expected_output.format(fn1, fn2, t1, t2)
 
 
-def test_git_diff_driver_flags(filespath, capsys, needs_git, reset_diff_targets):
+def test_git_diff_driver_flags(filespath, capsys, needs_git, reset_notebook_diff):
     # Simulate a call from `git diff` to check basic driver functionality
 
     fn1 = pjoin(filespath, 'foo--1.ipynb')
@@ -229,7 +229,7 @@ def test_git_diff_driver_flags(filespath, capsys, needs_git, reset_diff_targets)
         assert cap_out == expected_source_only.format(fn1, fn2, t1, t2)
 
 
-def test_git_diff_driver_ignore_flags(filespath, capsys, needs_git, reset_diff_targets):
+def test_git_diff_driver_ignore_flags(filespath, capsys, needs_git, reset_notebook_diff):
     # Simulate a call from `git diff` to check basic driver functionality
 
     fn1 = pjoin(filespath, 'foo--1.ipynb')
@@ -264,7 +264,7 @@ def _config_filter_driver(name, capsys):
         call('git config --local --add filter.%s.smudge "%s smudge"' % (name, base_cmd))
 
 
-def test_git_diff_driver_noop_filter(git_repo, filespath, capsys):
+def test_git_diff_driver_noop_filter(git_repo, filespath, capsys, reset_notebook_diff):
     _config_filter_driver('noop', capsys)
     fn1 = pjoin(git_repo, 'diff.ipynb')
     fn2 = pjoin(filespath, 'src-and-output--1.ipynb')
@@ -287,7 +287,7 @@ def test_git_diff_driver_noop_filter(git_repo, filespath, capsys):
         assert cap_out == expected_no_filter.format(fn1, fn2, t1, t2)
 
 
-def test_git_diff_driver_strip_outputs_filter(git_repo, filespath, capsys):
+def test_git_diff_driver_strip_outputs_filter(git_repo, filespath, capsys, reset_notebook_diff):
     _config_filter_driver('strip_outputs', capsys)
     fn1 = pjoin(git_repo, 'diff.ipynb')
     fn2 = pjoin(filespath, 'src-and-output--1.ipynb')
@@ -309,7 +309,7 @@ def test_git_diff_driver_strip_outputs_filter(git_repo, filespath, capsys):
         assert cap_out == expected_strip_output_filter.format(fn1, fn2, t1, t2)
 
 
-def test_git_diff_driver_add_helper_filter(git_repo, filespath, capsys):
+def test_git_diff_driver_add_helper_filter(git_repo, filespath, capsys, reset_notebook_diff):
     _config_filter_driver('add_helper', capsys)
     fn1 = pjoin(git_repo, 'diff.ipynb')
     fn2 = pjoin(filespath, 'src-and-output--1.ipynb')
@@ -332,7 +332,7 @@ def test_git_diff_driver_add_helper_filter(git_repo, filespath, capsys):
         assert cap_out == expected_helper_filter.format(fn1, fn2, t1, t2)
 
 
-def test_git_diff_driver_no_filter_without_flag(git_repo, filespath, capsys):
+def test_git_diff_driver_no_filter_without_flag(git_repo, filespath, capsys, reset_notebook_diff):
     _config_filter_driver('add_helper', capsys)
     fn1 = pjoin(git_repo, 'diff.ipynb')
     fn2 = pjoin(filespath, 'src-and-output--1.ipynb')
@@ -355,7 +355,7 @@ def test_git_diff_driver_no_filter_without_flag(git_repo, filespath, capsys):
 
 
 @pytest.mark.timeout(timeout=WEB_TEST_TIMEOUT)
-def test_git_web_diff_driver(filespath, unique_port, reset_log, ioloop_patch):
+def test_git_web_diff_driver(filespath, unique_port, reset_log, ioloop_patch, reset_notebook_diff):
     # Simulate a call from `git diff` to check basic driver functionality
 
     fn1 = os.path.join(filespath, 'foo--1.ipynb')

nbdime/tests/test_hg_differ.py

@@ -67,7 +67,7 @@
 """
 
 
-def test_hg_diff_driver(filespath, capsys, needs_hg):
+def test_hg_diff_driver(filespath, capsys, needs_hg, reset_notebook_diff):
     # Simulate a call from `hg diff` to check basic driver functionality
 
     fn1 = pjoin(filespath, 'foo--1.ipynb')
@@ -87,7 +87,7 @@ def test_hg_diff_driver(filespath, capsys, needs_hg):
         assert cap_out == expected_output.format(fn1, fn2, t1, t2)
 
 
-def test_hg_diff_driver_flags(filespath, capsys, needs_hg, reset_diff_targets):
+def test_hg_diff_driver_flags(filespath, capsys, needs_hg, reset_notebook_diff):
     # Simulate a call from `hg diff` to check basic driver functionality
 
     fn1 = pjoin(filespath, 'foo--1.ipynb')
@@ -107,7 +107,7 @@ def test_hg_diff_driver_flags(filespath, capsys, needs_hg, reset_diff_targets):
         assert cap_out == expected_source_only.format(fn1, fn2, t1, t2)
 
 
-def test_hg_diff_driver_ignore_flags(filespath, capsys, needs_hg, reset_diff_targets):
+def test_hg_diff_driver_ignore_flags(filespath, capsys, needs_hg, reset_notebook_diff):
     # Simulate a call from `hg diff` to check basic driver functionality
 
     fn1 = pjoin(filespath, 'foo--1.ipynb')
@@ -129,7 +129,7 @@ def test_hg_diff_driver_ignore_flags(filespath, capsys, needs_hg, reset_diff_tar
 
 
 @pytest.mark.timeout(timeout=WEB_TEST_TIMEOUT)
-def test_hg_web_diff_driver(filespath, unique_port, reset_log, ioloop_patch):
+def test_hg_web_diff_driver(filespath, unique_port, reset_log, ioloop_patch, reset_notebook_diff):
     # Simulate a call from `hg diff` to check basic driver functionality
 
     fn1 = os.path.join(filespath, 'foo--1.ipynb')

nbdime/tests/test_merge_notebooks.py

@@ -898,7 +898,7 @@ def test_autoresolve_empty_strategies():
     _check(partial, expected_partial, decisions, expected_conflicts)
 
 
-def test_only_sources(db, reset_diff_targets, reset_log):
+def test_only_sources(db, reset_notebook_diff, reset_log):
     base = db["mixed-conflicts--1"]
     local = db["mixed-conflicts--2"]
     remote = db["mixed-conflicts--3"]
@@ -916,7 +916,7 @@ def test_only_sources(db, reset_diff_targets, reset_log):
         assert len(path) == 2 or path[2] == 'source'
 
 
-def test_only_outputs(db, reset_diff_targets):
+def test_only_outputs(db, reset_notebook_diff):
     base = db["mixed-conflicts--1"]
     local = db["mixed-conflicts--2"]
     remote = db["mixed-conflicts--3"]
@@ -934,7 +934,7 @@ def test_only_outputs(db, reset_diff_targets):
         assert len(path) == 2 or path[2] == 'outputs'
 
 
-def test_only_metadata(db, reset_diff_targets):
+def test_only_metadata(db, reset_notebook_diff):
     base = db["mixed-conflicts--1"]
     local = db["mixed-conflicts--2"]
     remote = db["mixed-conflicts--3"]