From c072e4ad1fd278868651edaaf9ac6d55b52cfcab Mon Sep 17 00:00:00 2001 From: Murilo Cunha Date: Wed, 12 Jan 2022 11:03:55 -0300 Subject: [PATCH 1/6] remove unused DiffJupyterNotebook definition --- databooks/conflicts.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/databooks/conflicts.py b/databooks/conflicts.py index 75be453f..7ff81974 100644 --- a/databooks/conflicts.py +++ b/databooks/conflicts.py @@ -3,12 +3,12 @@ from __future__ import annotations from pathlib import Path -from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, cast +from typing import Any, Callable, List, Optional, Sequence, Tuple, cast from git import Repo from databooks.common import write_notebook -from databooks.data_models.base import BaseCells, DiffModel +from databooks.data_models.base import DiffModel from databooks.data_models.notebook import Cell, Cells, JupyterNotebook from databooks.git_utils import ConflictFile, get_conflict_blobs, get_repo from databooks.logging import get_logger, set_verbose @@ -16,15 +16,6 @@ logger = get_logger(__file__) -class DiffJupyterNotebook(DiffModel): - """Protocol for mypy static type checking.""" - - nbformat: int - nbformat_minor: int - metadata: Dict[str, Any] - cells: BaseCells[Any] - - def path2conflicts( nb_paths: List[Path], repo: Optional[Repo] = None ) -> List[ConflictFile]: From 998752ee120bdf4bdb449e7719aaced2402127eb Mon Sep 17 00:00:00 2001 From: Murilo Cunha Date: Wed, 12 Jan 2022 11:38:41 -0300 Subject: [PATCH 2/6] change type hints to return casted DiffModel (dynamic model)` --- databooks/conflicts.py | 10 +--------- databooks/data_models/base.py | 8 ++++---- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/databooks/conflicts.py b/databooks/conflicts.py index 7ff81974..8c2a9a10 100644 --- a/databooks/conflicts.py +++ b/databooks/conflicts.py @@ -8,7 +8,6 @@ from git import Repo from databooks.common import write_notebook -from databooks.data_models.base import DiffModel from databooks.data_models.notebook import Cell, Cells, JupyterNotebook from databooks.git_utils import ConflictFile, get_conflict_blobs, get_repo from databooks.logging import get_logger, set_verbose @@ -76,14 +75,7 @@ def conflict2nb( ) logger.debug(msg) - if cell_fields_ignore: - for cells in (nb_1.cells, nb_2.cells): - for cell in cells: - cell.clear_metadata( - cell_metadata_remove=[], cell_remove_fields=cell_fields_ignore - ) - - diff_nb = cast(DiffModel, nb_1 - nb_2) + diff_nb = nb_1 - nb_2 nb = cast( JupyterNotebook, diff_nb.resolve( diff --git a/databooks/data_models/base.py b/databooks/data_models/base.py index e4e7f6f7..d59a3834 100644 --- a/databooks/data_models/base.py +++ b/databooks/data_models/base.py @@ -134,7 +134,7 @@ def __str__(self) -> str: """Return outputs of __repr__.""" return repr(self) - def __sub__(self, other: DatabooksBase) -> DatabooksBase: + def __sub__(self, other: DatabooksBase) -> DiffModel: """ Subtraction between `databooks.data_models.base.DatabooksBase` objects. @@ -166,11 +166,11 @@ def __sub__(self, other: DatabooksBase) -> DatabooksBase: fields_d[name] = (tuple, (self_val, other_val)) # Build Pydantic models dynamically - DiffModel = create_model( + DiffInstance = create_model( "Diff" + type(self).__name__, __base__=type(self), resolve=resolve, is_diff=True, - **cast(Dict[str, Any], fields_d), + **fields_d, ) - return DiffModel() # it'll be filled in with the defaults + return cast(DiffModel, DiffInstance()) # it'll be filled in with the defaults From 1b25354c5b1dfd75b9846b25fd9793841a9b74ab Mon Sep 17 00:00:00 2001 From: Murilo Cunha Date: Wed, 12 Jan 2022 11:39:50 -0300 Subject: [PATCH 3/6] avoid using `cast` unnecessarily - prefer `foo: type = value` notation or checking types with `isinstance` --- databooks/data_models/base.py | 4 ++-- databooks/git_utils.py | 10 +++++++--- tests/test_conflicts.py | 4 ++-- tests/test_data_models/test_notebook.py | 3 +-- tests/test_git_utils.py | 12 +++++++++--- 5 files changed, 21 insertions(+), 12 deletions(-) diff --git a/databooks/data_models/base.py b/databooks/data_models/base.py index d59a3834..dc2cd7e6 100644 --- a/databooks/data_models/base.py +++ b/databooks/data_models/base.py @@ -83,7 +83,7 @@ def resolve( if not is_diff: raise TypeError("Can only resolve dynamic 'diff models' (when `is_diff=True`).") - res_vals = cast(Dict[str, Any], {}) + res_vals: Dict[str, Any] = {} for name, value in field_d.items(): if isinstance(value, (DiffModel, BaseCells)): res_vals[name] = value.resolve( @@ -152,7 +152,7 @@ def __sub__(self, other: DatabooksBase) -> DiffModel: other_d = dict(other) # Build dict with {field: (type, value)} for each field - fields_d = {} + fields_d: Dict[str, Any] = {} for name in self_d.keys() | other_d.keys(): self_val = self_d.get(name) other_val = other_d.get(name) diff --git a/databooks/git_utils.py b/databooks/git_utils.py index aa57d8fa..e2ba9fc3 100644 --- a/databooks/git_utils.py +++ b/databooks/git_utils.py @@ -1,7 +1,7 @@ """Git helper functions.""" from dataclasses import dataclass from pathlib import Path -from typing import Dict, List, cast +from typing import Dict, List from git import Blob, Git, Repo # type: ignore @@ -55,8 +55,12 @@ def get_conflict_blobs(repo: Repo) -> List[ConflictFile]: for k, v in unmerged_blobs.items() if 0 not in dict(v).keys() # only get blobs that could not be merged ) - # Type checking: `repo.working_dir` is not None if repo is passed - repo.working_dir = cast(str, repo.working_dir) + + if not isinstance(repo.working_dir, (Path, str)): + raise RuntimeError( + "Expected `repo` to be `pathlib.Path` or `str`, got" + f" {type(repo.working_dir)}." + ) return [ ConflictFile( filename=repo.working_dir / blob.filename, diff --git a/tests/test_conflicts.py b/tests/test_conflicts.py index dfdf5e47..1e3eba7e 100644 --- a/tests/test_conflicts.py +++ b/tests/test_conflicts.py @@ -1,5 +1,4 @@ from pathlib import Path -from typing import cast from py._path.local import LocalPath @@ -38,7 +37,8 @@ def test_path2diff(tmpdir: LocalPath) -> None: commit_message_main="Commit message from main", commit_message_other="Commit message from other", ) - git_repo.working_dir = cast(Path, git_repo.working_dir) + + assert isinstance(git_repo.working_dir, (Path, str)) conflict_files = path2conflicts(nb_paths=[nb_filepath], repo=git_repo) diff --git a/tests/test_data_models/test_notebook.py b/tests/test_data_models/test_notebook.py index abf5682c..3747b0e3 100644 --- a/tests/test_data_models/test_notebook.py +++ b/tests/test_data_models/test_notebook.py @@ -2,12 +2,11 @@ import logging from copy import deepcopy from importlib import resources -from typing import List, Tuple, cast +from typing import List, Tuple import pytest from _pytest.logging import LogCaptureFixture -from databooks.data_models.base import DiffModel from databooks.data_models.notebook import ( Cell, CellMetadata, diff --git a/tests/test_git_utils.py b/tests/test_git_utils.py index e9b0f31b..1e2f6a76 100644 --- a/tests/test_git_utils.py +++ b/tests/test_git_utils.py @@ -1,5 +1,4 @@ from pathlib import Path -from typing import cast from git import GitCommandError, Repo from py._path.local import LocalPath @@ -18,7 +17,12 @@ def init_repo_conflicts( ) -> Repo: """Create git repo and create a conflict.""" git_repo = Repo.init(path=tmpdir) - git_repo.working_dir = cast(Path, git_repo.working_dir) + + if not isinstance(git_repo.working_dir, (Path, str)): + raise RuntimeError( + f"Expected `pathlib.Path` or `str`, got {type(git_repo.working_dir)}." + ) + git_filepath = git_repo.working_dir / filename git_repo.git.checkout("-b", "main") @@ -64,7 +68,9 @@ def test_get_conflict_blobs(tmpdir: LocalPath) -> None: commit_message_main="Commit message from main", commit_message_other="Commit message from other", ) - git_repo.working_dir = cast(Path, git_repo.working_dir) + + assert isinstance(git_repo.working_dir, (Path, str)) + conflicts = get_conflict_blobs(repo=git_repo) assert len(conflicts) == 1 From 91c79ac80f587f7731c31fb5a9804fbad5b140dd Mon Sep 17 00:00:00 2001 From: Murilo Cunha Date: Wed, 12 Jan 2022 11:42:59 -0300 Subject: [PATCH 4/6] remove unnecessary casting --- tests/test_data_models/test_base.py | 6 ++---- tests/test_data_models/test_notebook.py | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/test_data_models/test_base.py b/tests/test_data_models/test_base.py index 66c5d481..a8349b99 100644 --- a/tests/test_data_models/test_base.py +++ b/tests/test_data_models/test_base.py @@ -1,13 +1,11 @@ -from typing import cast - -from databooks.data_models.base import DatabooksBase, DiffModel +from databooks.data_models.base import DatabooksBase def test_base_sub() -> None: """Use the `-` operator and resolve the diffs from the base model.""" model_1 = DatabooksBase(test=0, foo=1, bar="2") model_2 = DatabooksBase(bar=4, foo=2, baz=2.3, test=0) - diff = cast(DiffModel, model_1 - model_2) + diff = model_1 - model_2 assert diff.__class__.__name__ == "DiffDatabooksBase" assert dict(diff) == { "is_diff": True, diff --git a/tests/test_data_models/test_notebook.py b/tests/test_data_models/test_notebook.py index 3747b0e3..893f7f2a 100644 --- a/tests/test_data_models/test_notebook.py +++ b/tests/test_data_models/test_notebook.py @@ -202,7 +202,7 @@ def test_notebook_sub(self) -> None: ) notebook_2.cells = notebook_2.cells + [extra_cell] - diff = cast(DiffModel, notebook_1 - notebook_2) + diff = notebook_1 - notebook_2 notebook = deepcopy(notebook_1) # add `tags` since we resolve with default `ignore_none = True` From e9c1e5c5dfd52d9d61cdec6e699b2abd9d17dbbc Mon Sep 17 00:00:00 2001 From: Murilo Cunha Date: Thu, 13 Jan 2022 08:00:40 -0300 Subject: [PATCH 5/6] use isinstance for `resolve` methods - output types in signature are superclass since output type is dynamic --- databooks/conflicts.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/databooks/conflicts.py b/databooks/conflicts.py index 8c2a9a10..f0f6ebf0 100644 --- a/databooks/conflicts.py +++ b/databooks/conflicts.py @@ -3,7 +3,7 @@ from __future__ import annotations from pathlib import Path -from typing import Any, Callable, List, Optional, Sequence, Tuple, cast +from typing import Any, Callable, List, Optional, Sequence, Tuple from git import Repo @@ -76,16 +76,16 @@ def conflict2nb( logger.debug(msg) diff_nb = nb_1 - nb_2 - nb = cast( - JupyterNotebook, - diff_nb.resolve( - ignore_none=ignore_none, - keep_first=meta_first, - keep_first_cells=cells_first, - first_id=conflict_file.first_log, - last_id=conflict_file.last_log, - ), + nb = diff_nb.resolve( + ignore_none=ignore_none, + keep_first=meta_first, + keep_first_cells=cells_first, + first_id=conflict_file.first_log, + last_id=conflict_file.last_log, ) + if not isinstance(nb, JupyterNotebook): + raise RuntimeError(f"Expected `databooks.JupyterNotebook`, got {type(nb)}.") + logger.debug(f"Resolved conflicts in {conflict_file.filename}.") return nb From 2316fa13dc9ece2612b25ab382e7de01da6480d8 Mon Sep 17 00:00:00 2001 From: Murilo Cunha Date: Thu, 13 Jan 2022 08:33:08 -0300 Subject: [PATCH 6/6] change mypy shield - wasnt rendered correctly in docs --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e9244694..ea56a06d 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ # databooks [![maintained by dataroots](https://dataroots.io/maintained-rnd.svg)](https://dataroots.io) [![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/psf/black) -[![Checked with mypy](http://www.mypy-lang.org/static/mypy_badge.svg)](http://mypy-lang.org/) +[![Checked with mypy](https://img.shields.io/badge/mypy-checked-1f5082.svg)](http://mypy-lang.org/) [![Codecov](https://codecov.io/github/datarootsio/databooks/main/graph/badge.svg)](https://github.com/datarootsio/databooks/actions) [![tests](https://github.com/datarootsio/databooks/actions/workflows/test.yml/badge.svg)](https://github.com/datarootsio/databooks/actions) [![Downloads](https://pepy.tech/badge/databooks)](https://pepy.tech/project/databooks)