lervag / apy

CLI script for interacting with local Anki collection
MIT License
243 stars 17 forks source link

chore: add incremental type checking with mypy #59

Closed denismaciel closed 1 year ago

denismaciel commented 1 year ago

The PR sets up mypy to be adopted incrementally in the code base.

The idea is: we make mypy ignore all files. We then annotate one file at a time. As we annotate files, we remove them from being ignored by mypy.

I have type-annotated the apy.config module while ignoring all other modules.

The workflow for annotating an extra file is:

  1. Remove the file from the ignore list in pyproject.toml
  2. Run mypy src to identify the violations
  3. Fix the violations
  4. Be happy and type-safe

Do you think this works for you, @lervag?

If you're happy with the approach, I will annotate/fix other files and add a type-check step to the Github Action.

Looking forward to your feedback.

Greetings from Hamburg!

denismaciel commented 1 year ago

@lervag I have addressed your comments and added mypy to CI.

I was also thinking we could make the cfg dictionary a dataclasses.dataclass eventually.

ckp95 commented 1 year ago

I was actually planning to make a PR with a refactor of the config to use a dataclass. It's cleaner because you see the fields and the types all in one place. type checker can work with it better than a heterogeneously typed dict.

ckp95 commented 1 year ago

the idea is something like this

import json
import os
from dataclasses import dataclass, field
from pathlib import Path
from typing import Optional, Any

from platformdirs import user_config_path, user_data_path

def env_path(name: str, must_exist_if_specified=True) -> Optional[Path]:
    if (raw_path := os.environ.get(name)) is None:
        return None

    if must_exist_if_specified and not (path := Path(raw_path)).exists():
        raise FileNotFoundError(f"No such path: {name}={path}")

    return path

def get_raw_config() -> dict[str, Any]:
    cfg_dir = env_path("APY_CONFIG") or user_config_path("apy")
    if not (cfg_path := cfg_dir / "apy.json").exists():
        return {}

    return json.loads(cfg_path.read_text())

def get_base_path() -> Path:
    return env_path("APY_BASE") or env_path("ANKI_BASE") or user_data_path("Anki2")

@dataclass
class Config:
    base_path: Path = field(default_factory=get_base_path)
    img_viewers: dict[str, list[str]] = field(
        default_factory=lambda: {"svg": ["display", "-density", "300"]}
    )
    img_viewers_default: list[str] = field(default_factory=lambda: ["feh"])
    markdown_models: list[str] = field(default_factory=lambda: ["Basic"])
    presets: dict[str, dict] = field(default_factory=dict)
    profile: Optional[str] = None
    query: str = "tag:marked OR -flag:0"

    def __post_init__(self) -> None:
        if not self.base_path.exists():
            raise FileNotFoundError(f"Could not find Anki base path: {self.base_path}")

        self.base_path = self.base_path.absolute()
        self.presets.setdefault("default", {"model": "Basic", "tags": []})

cfg = Config(**get_raw_config())

haven't tested this or modified the other modules to account for the changes yet.

ckp95 commented 1 year ago

Would it make more sense for me to push this dataclass config stuff to this PR, or to make a separate one?

lervag commented 1 year ago

Could you make a new PR? I'll merge this one probably later today. If you make the PR based on this branch and later rebase it on master after this is merged, then it should be quite clean.

lervag commented 1 year ago

I've merged this now; thanks!