Skip to content

Conversation

@dmacvicar
Copy link

I was writing my own importer and could not get extract --reverse to work, until I confirmed I was getting reverse as False all the time.

You can reproduce this with this test:

diff --git a/beangulp/extract_test.py b/beangulp/extract_test.py
index ceaa443..25f6cdf 100644
--- a/beangulp/extract_test.py
+++ b/beangulp/extract_test.py
@@ -1,4 +1,5 @@
 import io
+import tempfile
 import textwrap
 import unittest
 
@@ -6,7 +7,10 @@ from datetime import timedelta
 from os import path
 from unittest import mock
 
+import click.testing
+
 from beancount.parser import parser
+import beangulp
 from beangulp import extract
 from beangulp import similar
 from beangulp import tests
@@ -53,6 +57,48 @@ class TestExtract(unittest.TestCase):
         with self.assertRaises(AssertionError):
             extract.extract_from_file(importer, path.abspath("test.csv"), [])
 
+    def test_extract_reverse_flag_passed_from_cli(self):
+        entries, errors, options = parser.parse_string(
+            textwrap.dedent("""
+            1970-01-03 * "Test"
+              Assets:Tests  1.00 USD
+
+            1970-01-01 * "Test"
+              Assets:Tests  1.00 USD
+            """)
+        )
+
+        class ReverseImporter(tests.utils.Importer):
+            def __init__(self):
+                super().__init__("test.Reverse", "Assets:Tests", None)
+                self.last_reverse = None
+
+            def identify(self, filepath):
+                return True
+
+            def extract(self, filepath, existing):
+                return list(entries)
+
+            def sort(self, entries, reverse=False):
+                self.last_reverse = reverse
+                return super().sort(entries, reverse=reverse)
+
+        importer = ReverseImporter()
+        ingest = beangulp.Ingest([importer])
+        runner = click.testing.CliRunner()
+
+        with tempfile.TemporaryDirectory() as tempdir:
+            filepath = path.join(tempdir, "test.txt")
+            with open(filepath, "w") as handle:
+                handle.write("fixture")
+
+            result = runner.invoke(
+                ingest.cli, ["extract", "--reverse", filepath], catch_exceptions=False
+            )
+
+        self.assertEqual(result.exit_code, 0)
+        self.assertTrue(importer.last_reverse)
+
 
 class TestDuplicates(unittest.TestCase):
     def test_mark_duplicate_entries(self):

Copy link
Collaborator

@dnicolodi dnicolodi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this bug survived so long. Thanks for the fix. Can you please add a test to the beangulp/tests/extract.rst doctest? The other tests there should be guidance enough to add a test for the sorting functionality.

filename,
existing_entries,
reverse=reverse,
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think passing reverse as a keyword argument is necessary here. Using a regular argument should make the function call fit on a single line and avoid the ugly black indentation style.

importer: The importer instance to handle the document.
filename: Filesystem path to the document.
existing_entries: Existing entries.
reverse: Sort extracted entries in descending order.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reverse: Sort extracted entries in descending order.
reverse: Sort extracted entries in reverse order.

Whether the order is ascending or descending and which filed is used for sorting depends entirely on the importer implementation.


# Sort the newly imported entries.
importer.sort(entries)
importer.sort(entries, reverse=reverse)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
importer.sort(entries, reverse=reverse)
importer.sort(entries, reverse)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants