Repository navigation
refactor: enforce maximum strictness across the codebase - #17
Merged
Merged
Conversation
Ruff selects ALL with complexity limits (mccabe 10, pylint defaults stated explicitly, four preview complexity rules opted in one by one) and bans relative imports; the only global ignores are COM812 (formatter conflict) and CPY001 (licensing lives in LICENSE). Google docstring convention. Tests may use asserts and literal expected values. ty runs every rule at error with strict equality and generic narrowing and ignores `type: ignore` comments. tests/test_repository_limits.py holds what no lint rule covers: modules stay within 600 lines, and every suppression names its rule and a reason. The tree does not pass these gates yet; follow-up commits refactor it.
Every command module created a Rich Console at import. A Console fixes its color system and terminal mode when constructed, so importing the app from a terminal (pytest -s, or a Nix check phase) kept ANSI codes in output later redirected into CliRunner: `script -q /dev/null uv run pytest -s` failed 29 tests. kicad_lib_manager.utils.console.get_console builds a console for the current stream on each call and every command uses it. main.py no longer installs rich.traceback at import (it also created a console then, and showed locals that Typer's pretty exceptions deliberately hide). The import-zip test helpers import the app at module level again. tests/test_console.py fails if any module holds a console and checks that get_console follows a redirected stdout.
Output of `ruff check --fix` (safe fixes only) and `ruff format` under the new rule set; no behavior change, the tests pass unchanged.
Resolves INP001 for every test module at once: pytest imports them as tests.<name>, so module names under tests/ cannot collide.
Modules that use questionary, requests, jinja2, yaml, pathspec, or packaging.version only inside functions declare them in __lazy_modules__ (PEP 810). Python 3.15 then defers the import until first use; older versions ignore the variable. Imports stay at module top level. `python -X importtime -c 'import kicad_lib_manager.main'` on Python 3.15.0, macOS arm64, median of five warm runs: 140 ms before, 45 ms after. None of the six modules is loaded by the import; `kilm --help` and the test suite pass on 3.15.
Adds 3.15 to the check matrix and classifiers. The matrix runs every registered command's --help through tests/test_leaf_command_usage.py and imports every module through tests/test_console.py, which exercises the PEP 810 lazy imports on 3.15.
- publish_release(github, publication) takes a frozen Publication instead of five keyword arguments (PLR0913). - gh JSON is parsed by parse_release and has_tag_ref, which check the shape and raise ReleaseError instead of trusting json.loads as a TypedDict (ty unsound-assignment). The asset rule (uploaded state and a sha256 digest) and the exact-tag rule for matching-refs now have tests. - GhCli resolves gh on PATH once and runs it by absolute path (S607); a missing gh is a ReleaseError instead of FileNotFoundError. - main writes the annotation and summary to stderr/stdout directly (T201). - Type-only imports under TYPE_CHECKING; test docstrings; the draft test parameter is keyword-only (FBT001). Behavior is otherwise unchanged. Read-only run of main against the real v0.5.6 release (writes replaced by failures) accepted it with no writes and rejected a foreign RELEASE_SHA.
Backups were named with a whole-second stamp, so a second backup of the same file in that second overwrote the first and the history kept fewer versions than max_backups. Stamps now carry microseconds, and a taken name moves the stamp forward a microsecond, so name order stays creation order (pruning relies on it). Old whole-second names sort correctly among the new ones. Closes #12
Rich wrapped long values at the console width, inserting newlines inside JSON strings so the output did not parse. Echo the JSON as is.
UV_TOOL_DIR or UV_TOOL_BIN_DIR made any install look like a uv tool, for example on CI runners and in shells that use uv for other tools. Only an interpreter inside UV_TOOL_DIR or uv's default tools directory (uv/tools, uv/data/tools on Windows) now counts.
CONDA_DEFAULT_ENV is set whenever a conda environment is active, and many shells activate base by default, so a uv tool or pip install of kilm was reported as conda and refused automatic updates. Only an interpreter inside CONDA_PREFIX or a conda directory now counts.
create accepted --library but ignored it, so a template that shared its name with one in an earlier library could not be used. list filtered after name clashes were resolved, so it hid the same template. Both now search only the requested library.
template make writes defaults such as directory_name and project_filename
in the %{...} filename syntax, but template create rendered only {{ ... }}
defaults. Accepting the offered default stored the raw expression, so
files named %{project_filename} were created as '%{project_name}.kicad_pro'
and content saw the unrendered text.
update_kicad_env_vars, update_pinned_libraries, and write_file_with_encoding (used for the library tables) truncated the file and then wrote it, so a failure part way (a full disk, or a library name carrying an undecodable byte as a lone surrogate) left KiCad's configuration empty or half-written. They now encode first and replace the file with write_atomically.
The prompt for a detected variable's description reused the name of the template description, so the last answer became the template's description in metadata.yaml.
… pip detect_installation_method returns an InstallMethod Literal. Interpreters in the Nix store are reported as nix with flake update and rebuild guidance; pip is reported only when kilm's INSTALLER record says pip, and anything else is unknown. Neither is offered pip, which cannot update a read-only store path or a distribution package.
The main project, schematic, and PCB were recorded as strings and compared with Path objects, which never matched. They were copied unchanged, so projects created from the template kept the source project's file names and references instead of project_filename.
select_autoescape escapes templates built from strings by default, so a project named 'R&D' became 'R&D' in KiCad files and Markdown. The templates produce KiCad and text files, never HTML.
fix_invalid_uris replaced `(uri "${/path}/lib")` with the literal text
`(uri "\1/\2")` because the raw replacement string doubled its backslashes,
so every repaired library lost its path. It also rewrote the table in place,
which leaves an empty table when the write fails part way. Use real group
references and utils.file_ops.write_atomically.
setup created the sym-lib-table and fp-lib-table backups after LibraryService.add_libraries had already written the new entries, so the backup held the changed table and the original was lost. Plan the change with a dry run, back up, then apply it.
unpin rewrote KiCad's kicad_common.json in place, so a failed write left an empty file and KiCad lost every setting in it. Write through utils.file_ops.write_atomically. The unpin tests now run against a kicad_common.json in tmp_path instead of patching builtins.open and Path.exists globally, and they assert the resulting file rather than accepting exit code 0 or 1.
`kilm sync --auto-setup` calls the setup function directly. setup declared its options as `param = typer.Option(...)` defaults, so the direct call received OptionInfo objects instead of None and False and failed with "'OptionInfo' object has no attribute 'split'". Declare the options with Annotated so the Python defaults are the CLI defaults. The sync tests now pull from real git repositories in tmp_path instead of patching Path methods and subprocess.run globally.
read/write/get_default for kilm.yaml and .kilm_metadata now use GitHubMetadata and CloudMetadata (all fields optional, as in the files). Reading checks the known fields: one with the wrong type reads as absent, unknown fields are kept so a read-change-write cycle preserves them. Files are read and written as UTF-8 (KiLM writes them as ASCII, so existing files read the same). Read and write errors go to stderr instead of stdout, and only the expected I/O and parse errors are caught. generate_env_var_name no longer raises IndexError for an empty name without prefix; its 50-character limit is MAX_ENV_VAR_NAME_LENGTH.
After a pull, sync compared HEAD~1 with HEAD, so a pull that brought several commits only reported the libraries added in the last one, and --auto-setup did not run when an earlier commit added a library. Compare with the HEAD from before the pull; check_for_library_changes takes that revision as an optional argument and keeps HEAD~1 as its default.
The command-wide `except Exception` also caught the typer.Exit raised after an error had been printed (click's Exit is a RuntimeError), so a directory that is not a repository printed a second, empty "Error: 1" line. Let Exit pass through. The hook was written in text mode, which gives the shell script CRLF line endings on Windows; write the bytes atomically instead.
UpdateService.perform_update dropped target_version, so an update always installed the latest release, and an up-to-date install refused to move to a requested older version. The installer now receives kilm==VERSION (pipx install --force, uv tool install, pip install --upgrade), the version is validated as a usage error, and asking for the installed version is a no-op without --force. A dry run on an installation method without automatic updates (conda, Homebrew, Nix, unknown) exited 0; it now exits 1 like the real update.
uv tool upgrade keeps an exact pin from an earlier install (for example uv tool install kilm==0.5.5 or kilm update perform --target-version) and exits 0 without changing anything, so kilm reported a successful update that did not happen. uv tool install kilm@latest replaces the pin; it is also a no-op on an unpinned install that is already current.
PyPIVersionChecker ignored its cache except for the ETag, and UpdateService.check_for_updates(use_cache=...) called the same method in both branches. Every kilm --help, kilm update check, and kilm update info therefore queried PyPI, and --force, --force-check, and --no-cache changed nothing. A cached answer younger than a day is now returned without a request unless the cache is bypassed; stale entries still supply the ETag. The cache file location is injectable so the tests no longer write to the real ~/.cache/kilm.
find_environment_variables tries one small lookup per source instead of one 20-branch function, catches only the errors each source can raise, and runs fish by its resolved path. The kicad_common.json updaters share load/save helpers; a null "vars" or pinned list (or a missing one) now reads as empty instead of raising TypeError, and a malformed file raises KiCadConfigError (a ValueError, like the invalid-JSON case). Duplicate names passed to update_pinned_libraries are pinned once. update_pinned_libraries takes its options by keyword, as every caller already does. update_kicad_env_vars keeps dry_run positional because setup and KiCadService pass it that way.
UpdateManager created its PyPIVersionChecker without the current version, so every request identified itself as KiLM/unknown.
…rm docstrings pypi_version caches under user_paths.user_cache_dir() instead of rebuilding the path. update perform help now covers --target-version and the installer fallback.
find_environment_variables becomes find_library_variable (it returns one value from config, cwd metadata, environment, and shell files), and setup's fallback and source bucket follow. Printed labels are unchanged.
Owner
Author
|
Refine pass (standards.md + AGENTS.md) pushed as 14 commits, 5d2dd02..6454876. Behavior is unchanged except one bug fix:
Refactors: one owner for kicad_common.json access ( Tests: one suite-wide Left as follow-ups because they change behavior:
|
Owner
Author
|
Review fixes (each with a failing test first, one commit each):
|
Reading .kilm_metadata dropped known fields of the wrong type (version: 2, description: null), and add-3d rewrote the file without them even when nothing was overridden. read_cloud_metadata_file returns them separately and write_cloud_metadata puts them back unless a field set by the caller replaces them; --force still starts fresh.
Custom keys were turned into strings, so 7: and '7': collapsed (also inside variables and dependencies) and a hook reading template[7] failed. Keys now stay as YAML loads them; discovery builds the template mapping instead of unpacking metadata as keyword arguments, and template list --json prints keys JSON cannot hold, such as dates, as text instead of failing.
The page listed a last-updated timestamp that the file never had.
find_potential_variables used the default patterns for patterns=[]; only None means the defaults now.
…e notes Plain kilm update only prints the subcommands.
Owner
Author
|
Review fixes pushed (head eb00241), each with a failing test first where it is a bug:
Left for a follow-up: the update check records a check only when it finds an update. Recording every check would not make the frequency setting work, because the not-due branch still refreshes the day-long PyPI cache. That needs a cache-only read.
|
This was referenced Oct 12, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enforces the strictest gates the tools offer and refactors the whole tree until it passes them, with no debt baseline.
just check(ruff format, ruff check, ty, tests) is green with zero violations.Closes #12.
Gates
select = ["ALL"]. The only ignores are COM812 (it conflicts with the formatter) and CPY001, each with a stated reason. Google docstring convention,ban-relative-imports = "all", mccabe 10, and the pylint size limits set explicitly. Preview rules PLR0904, PLR0914, PLR0916 and PLR1702 are enabled.respect-type-ignore-comments = false, strict equality semantics and strict generic narrowing.Anyis gone from the package.tests/test_repository_limits.pyadds two checks: modules stay within 600 lines, and everynoqaorty: ignorenames its rule and gives a reason after--.type: ignoreis banned. A third check rejects emoji and check-mark glyphs in Python modules. Fifteen suppressions remain, each with a reason: subprocess calls with fixed arguments, Typer commands with many options,autoescape=Falsefor KiCad output, and the catch-all around user template hooks.Per-area changes
utils.console.get_console()builds a console per call. Consoles fixed at import time broke tests under Nix and in terminals.rich.traceback.installis removed.__lazy_modules__(PEP 810) defers questionary, requests, jinja2, yaml, pathspec and packaging.version.import kicad_lib_manager.mainon Python 3.15 (macOS arm64, warm) took 137-144 ms before and takes 46-48 ms now.tests/test_startup_imports.pychecks on 3.15 that none of these modules load at import.GitHubMetadata,CloudMetadata) that are validated on read, and errors go to stderr.LibraryTableEntry.env_varsis split, withKiCadConfigErroradded.import_zipis restructured, andImportFailureis renamedImportZipError.install_method,pypi_version,update_service,user_paths,config_service,kicad_config,library_table,library_serviceandkicad_service.Config(*, config_file=None, cache_dir=None)is typed, and loading it no longer creates~/.config.interfaces/package, dead service methods and theConfigServiceshim are deleted.setupis split intocommand.py,sources.pyandchanges.py. Its output is identical in 17 recorded scenarios.commands/shared.pyandcommands/kicad_common.py, with typed access tokicad_common.json.Annotatedoptions and keyword-only parameters.main.pyusesConfig()and detects a run with no arguments from the Click context.typing-extensionsis now a direct dependency (for@overrideon 3.10).utils/template.pybecomes theutils/template/package: models, rendering, discovery, selection, kicad_files, authoring, hooks and project.create.py,make.pyandlisting.py.plan_template_filesis now the only place that decides whatmakecopies, previews and scans..github/scripts/github_release.pychecks the shape of gh's JSON output.publish_releasetakes aPublication.ghis looked up once, and a missingghraisesReleaseError.~/.cache/kilm, the real config and PyPI.tests/fault_injection.pysimulates failed writes.Bug fixes folded in
Each fix has its own commit and starts with a failing test.
kicad_common.jsonand the library tables are replaced atomically.setupbacks up the tables before adding libraries.unpinwrites atomically.\1/\2and destroyed wrapped URIs.--max-backupswas ignored when pinning.--auto-setupcrashed.Error: 1.--target-versionis now honored by a real update. Before, only--dry-runused it.--no-cachedid nothing.list --jsonwrapped long lines.--librarywas ignored bycreateandlist.%{...}defaults were shown unrendered.makeoverwrote the template description.makenever templated the main KiCad files.make --dry-runpreview disagreed with the actual copy.post_createhook was a SyntaxError.makeoption that does not exist are removed fromlist.listnamed a nonexistentkilm list-libraries; they now namekilm config list.initreported folders it had just created as already existing.initignored a failure to writekilm.yaml.config removeof the default library left a dangling default.config list --typegave no explanation when a type had no libraries.add-hookreported a failure twice.statusshowsCurrentandYes/Missinginstead of check marks.add-3dandsetupdocs give the metadata file as.kilm_metadata(they said.kilm_cloud_metadata).Verification
just check: 376 passed, 1 skipped (the 3.15-only startup test).just cov: 76.6%.script -q /dev/null just test): passed.nix flake check -L: passed.just docs: built 25 pages.--help,--version, a run with no arguments,setupandstatusmatched the base commit in recorded scenarios.No version change; releasing is a separate step.