types #143

yagebu · 2025-01-03T07:38:10Z

adjust types for a successful mypy run
ci: add mypy ci step

Some type: ignores were added but only internal uses of functionality.

dnicolodi

I'm not convinced that adding types to the examples is a good idea. The examples are supposed to be as simple as possible and serve as templates for inexperienced programmers. Adding typing annotations seems to only add complexity for very little gain.

dnicolodi · 2025-01-11T21:41:51Z

.github/workflows/lint.yaml

+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: '3.9'


Why Python 3.9?

dnicolodi · 2025-01-11T21:42:34Z

beangulp/__init__.py

@@ -257,7 +258,7 @@ def _importer(importer):


 class Ingest:
-    def __init__(self, importers, hooks=None):
+    def __init__(self, importers: Sequence[Union[Importer, ImporterProtocol]], hooks: Optional[Sequence[Any]] = None) -> None:


hooks: Optional[Sequence[Any]] could at least be hooks: Optional[Sequence[Callable]]

dnicolodi · 2025-01-11T21:42:54Z

beangulp/cache.py

@@ -155,7 +155,7 @@ def get_file(filename):
    return _CACHE[filename]


-_CACHE = utils.DefaultDictWithKey(_FileMemo)
+_CACHE = utils.DefaultDictWithKey(_FileMemo)  # type: ignore


Why is the ignore needed?

dnicolodi · 2025-01-11T21:44:18Z

beangulp/importer.py

        """See Importer class name property."""
        return f"{self.__class__.__module__}.{self.__class__.__name__}"

    __str__ = name

    def identify(self, file) -> bool:
        """See Importer class identify() method."""
+        # Type error ignore for backwards compatibility - this should be overridden
+        # and implemented in subclasses
+        return None  # type: ignore


If returning None is not allowed, return None should be raise NotImplementedError. If it is allowed, the annotation is wrong.

dnicolodi · 2025-01-11T21:44:33Z

beangulp/importer.py


    def file_account(self, file) -> data.Account:
        """See Importer class account() method."""
+        # Type error ignore for backwards compatibility - this should be overridden
+        # and implemented in subclasses
+        return None  # type: ignore


dnicolodi · 2025-01-11T21:51:04Z

examples/importers/csvbank.py

@@ -5,7 +5,7 @@


 class Importer(csvbase.Importer):
-    date = csvbase.Date('Posting Date', '%m/%d/%Y')
+    date = csvbase.Date('Posting Date', '%m/%d/%Y')  # type: ignore


dnicolodi · 2025-01-11T21:51:53Z

examples/importers/utrade.py

        index = 0
        with open(filepath) as infile:
            for index, row in enumerate(csv.DictReader(infile)):
                meta = data.new_metadata(filepath, index)
                date = parse(row['DATE']).date()
                rtype = row['TYPE']
                link = f"ut{row['REF #']}"
+                links = frozenset([link])


These two lines can be one line.

dnicolodi · 2025-01-11T21:54:45Z

examples/importers/utrade.py

@@ -122,9 +123,9 @@ def extract(self, filepath, existing):
                    rate = D(match.group(3))

                    if rtype == 'BUY':
-                        cost = position.Cost(rate, self.currency, None, None)
+                        cost = position.CostSpec(rate, None, self.currency, None, None, None)


Why? Cost should be just fine.

dnicolodi · 2025-01-11T21:54:55Z

examples/importers/utrade.py

@@ -143,11 +144,11 @@ def extract(self, filepath, existing):
                            logging.error("Missing cost basis in '%s'", row['DESCRIPTION'])
                            continue
                        cost_number = D(match.group(1))
-                        cost = position.Cost(cost_number, self.currency, None, None)
+                        cost = position.CostSpec(cost_number, None, self.currency, None, None, None)


dnicolodi · 2025-01-11T21:56:27Z

beangulp/petl_utils.py

+            links.add(link)
+        tag = getattr(rec, "tag", None)
+        if tag:
+            tags.add(tag)


I'm not entirely convinced that the type for links and tags should be frozenset and not simply collections.abc.Set. Anyhow, This seems a very convoluted way to crete a frozenset of one element.

yagebu force-pushed the types branch from bb2f4ce to 2aec7f8 Compare January 3, 2025 07:44

yagebu added 2 commits January 3, 2025 08:48

adjust types for a successful mypy run

4b45546

Some type: ignores were added but only internal uses of functionality.

ci: add mypy ci step

cc708dd

yagebu force-pushed the types branch 2 times, most recently from f4c3b0f to c4138e4 Compare January 3, 2025 08:03

use types in some of the examples importers

0b55392

yagebu force-pushed the types branch from c4138e4 to 0b55392 Compare January 3, 2025 08:06

yagebu marked this pull request as ready for review January 3, 2025 08:18

yagebu mentioned this pull request Jan 3, 2025

add py.typed marker #141

Open

dnicolodi requested changes Jan 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types #143

types #143

yagebu commented Jan 3, 2025

dnicolodi left a comment

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

dnicolodi Jan 11, 2025

types #143

Are you sure you want to change the base?

types #143

Conversation

yagebu commented Jan 3, 2025

dnicolodi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment