codexctl icon indicating copy to clipboard operation
codexctl copied to clipboard
verified publisher icon Jayy001

Make pyright mostly happy

Open Eeems opened this issue 5 months ago • 2 comments

@Jayy001 I've gone through with a LSP and addressed as much of the warnings/errors it was raising about incorrect/unknown types as possible. This found a couple method signatures that were incorrect, several places where None could be returned that were not being handled properly, and other issues. There are still some warnings/errors that probably should be addressed, but I'm done for now. I have not done any testing of this, so I would highly recommend doing so as part of reviewing the changes.

I do think that some work needs to be done to expand the automated testing to cover more of the possible functions, as well as better test the various branching paths, as there are various open issues about codepaths that have just never worked, so they were never tested.

Summary by CodeRabbit

  • New Features

    • Added a CLI "list" subcommand for viewing available versions.

  • Refactor

    • Improved type safety and input validation across the tool.
    • More robust update/version handling and engine compatibility checks.
    • Safer update install workflow with better file-management and cleanup.
    • Enhanced sync and server communication reliability with stricter response validation.

  • Style

    • Cleaned up imports and standardized ignoring of non-essential return values.

  • Documentation

    • Updated internal docs to reflect API and behavior refinements.

Eeems avatar Jul 21 '25 05:07 Eeems

Walkthrough

Adds extensive static typing and type-safety across modules, removes unused XML/update code, tightens error handling, reorganizes some imports, standardizes ignoring of side-effect return values, and extends CLI/update workflows (including a new list subcommand and version resolution/installation logic).

Changes

Cohort / File(s) Change Summary
Core init / CLI
codexctl/__init__.py		 Reordered imports, added public exports (HardwareType, UpdateManager), strengthened type annotations (Manager fields and call_func), deferred DeviceManager import, expanded update/version resolution and install workflow, added list subcommand, and standardized discarding of move/write return values.
Analysis
codexctl/analysis.py		 Added return type annotation: get_update_image(file: str) -> tuple[UpdateImage, ext4.Volume]; trivial import formatting.
Device management
codexctl/device.py		 Wide typing additions for DeviceManager (constructor, methods, attributes), changed get_host_address to return single str, updated signatures (connect_to_device, get_device_status, edit_update_conf, install_ohma_update, etc.), hardware-aware restore logic, and standardized ignoring of FS/system call results.
Server / HTTP update host
codexctl/server.py		 Tightened function signatures (getupdateinfo, scanUpdates, startUpdate), removed unused params, safer header/tag handling in do_POST, explicit typing of return values and discarding of wfile write results.
Sync / RmWeb API
codexctl/sync.py		 Added typing across RmWebInterfaceAPI (constructor, __POST, doc/folder methods, download, upload, sync), response validation (type checks/casts), and standardized ignoring of file-write return values.
Update manager
codexctl/updates.py		 Introduced typed version maps and attributes, stronger return types for get_remarkable_versions, removed XML-generation/parsing helpers, hardened download (__download_version_file) types and parsing, improved error handling/exception chaining, and Path-compatible download handling.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User
    participant CLI
    participant Manager
    participant UpdateMgr
    participant DeviceMgr

    User->>CLI: invoke subcommand (e.g. update/list/install)
    CLI->>Manager: call_func(function, args)
    Manager->>UpdateMgr: resolve/version selection (latest/toltec/regex)
    alt version requires device interaction
        Manager->>DeviceMgr: connect (address, password)
        DeviceMgr-->>Manager: device status (version, engine type)
        UpdateMgr->>DeviceMgr: download/install update files
        DeviceMgr-->>UpdateMgr: install result
    else purely server-side list
        UpdateMgr-->>Manager: list of versions
    end
    Manager-->>CLI: result/exit
    CLI-->>User: output

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pre-merge checks (2 passed, 1 inconclusive)

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The title “Make pyright mostly happy” alludes to resolving type‐checker issues but uses informal language and doesn’t clearly describe the substantive changes—widespread addition of static type annotations to address Pyright warnings—and the qualifier “mostly” adds ambiguity about completion. Consider renaming the title to more precisely and professionally convey the core work, for example “Add static type annotations and resolve Pyright warnings,” which clearly indicates that the PR focuses on typing improvements to satisfy the Pyright checker.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

[!TIP]

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing Touches
  • [ ] 📝 Generate Docstrings
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch typing

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Jul 21 '25 05:07 coderabbitai[bot]

It would probably be worth implementing the out of range comment

Eeems avatar Oct 21 '25 14:10 Eeems