Skip to content

Make pyright mostly happy #140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Eeems wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Make pyright mostly happy #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ebb4e6
Make pyright mostly happy
Eeems Jul 21, 2025
188c18e
Address code review comments
Eeems Aug 2, 2025
69cbe4f
More code review fixes
Eeems Aug 2, 2025
daf9c51
Remove unused imports
Eeems Aug 2, 2025
79e0102
Merge branch 'main' into typing
Jayy001 Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 47 additions & 41 deletions codexctl/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
import importlib.util
import tempfile
import shutil
import json
import re

from typing import cast
from os import listdir
from typing import Any, Callable, cast
from .device import HardwareType

from .updates import UpdateManager

try:
from loguru import logger
Expand All @@ -23,10 +24,6 @@
"Requests is required for accessing remote files. Please install it."
)

from .device import HardwareType
from .updates import UpdateManager


class Manager:
"""
Main class for codexctl
Expand All @@ -39,11 +36,11 @@ def __init__(self, device: str, logger: logging.Logger) -> None:
device (str): Type of device that is running the script
logger (logger): Logger object
"""
self.device = device
self.logger = logger
self.updater = UpdateManager(logger)
self.device: str = device
self.logger: logging.Logger = logger
self.updater: UpdateManager = UpdateManager(logger)

def call_func(self, function: str, args: dict) -> None:
def call_func(self, function: str, args: dict[str, Any]) -> None:
"""Runs a command based on the function name and arguments provided

Args:
Expand All @@ -55,9 +52,9 @@ def call_func(self, function: str, args: dict) -> None:
remarkable_version = HardwareType.parse(self.device)
except ValueError:
hw = args.get("hardware")
remarkable_version = HardwareType.parse(hw) if hw else None
remarkable_version = cast(str, HardwareType.parse(hw)) if hw else None
Comment on lines 54 to +55
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Type casting issue with HardwareType.parse

HardwareType.parse returns a HardwareType enum, not a string. The cast is incorrect.

             hw = args.get("hardware")
-            remarkable_version = cast(str, HardwareType.parse(hw)) if hw else None
+            remarkable_version = HardwareType.parse(hw) if hw else None
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
hw = args.get("hardware")
remarkable_version = HardwareType.parse(hw) if hw else None
remarkable_version = cast(str, HardwareType.parse(hw)) if hw else None
hw = args.get("hardware")
remarkable_version = HardwareType.parse(hw) if hw else None
🤖 Prompt for AI Agents 
In codexctl/__init__.py around lines 54-55, the code incorrectly casts the
result of HardwareType.parse to str; HardwareType.parse returns a HardwareType
enum. Replace the cast and assign the enum directly (remarkable_version =
HardwareType.parse(hw) if hw else None). If the downstream code actually
requires a string, explicitly convert the enum to the desired string form (e.g.,
.value or .name) instead of using cast.

Copy link
Collaborator Author

@Eeems Eeems Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's being used as a string later in the code.

Copy link
Contributor

@coderabbitai coderabbitai bot Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the HardwareType.parse method signature and return type
ast-grep --pattern 'class HardwareType {
  $$$
  def parse($$$) {
    $$$
  }
  $$$
}'

# Also search for HardwareType definition and parse method
rg -n "class HardwareType" --type=py
rg -n "def parse" --type=py -A 10 -B 2

Length of output: 831

🏁 Script executed:

#!/bin/bash
# Check what UpdateManager methods expect for hardware type parameter
ast-grep --pattern 'class UpdateManager {
  $$$
  def get_latest_version($$$) {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class UpdateManager {
  $$$
  def get_toltec_version($$$) {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class UpdateManager {
  $$$
  def download_version($$$) {
    $$$
  }
  $$$
}'

# Also check the method signatures directly
rg -n "def get_latest_version\|def get_toltec_version\|def download_version" --type=py -A 3 -B 1

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Search for UpdateManager class and its methods more broadly
rg -n "class UpdateManager" --type=py -A 20

# Search for specific method definitions in updates.py
rg -n "get_latest_version\|get_toltec_version\|download_version" codexctl/updates.py -A 5 -B 2

Length of output: 1294

🏁 Script executed:

#!/bin/bash
# Search specifically for the method definitions we need
fd "updates.py" --exec cat {} | grep -n -A 10 "def get_latest_version\|def get_toltec_version\|def download_version"

Length of output: 1321

🏁 Script executed:

#!/bin/bash
# Check how remarkable_version is used later in the code, particularly in device.py
rg -n "remarkable.hardware" codexctl/__init__.py -A 2 -B 2

Length of output: 751

@coderabbit-user Thank you for the clarification! You're absolutely right. After analyzing the code, I can see that the UpdateManager methods (get_latest_version, get_toltec_version, and download_version) all expect a HardwareType enum as their first parameter, not a string.

However, I notice there's an inconsistency in the code. The remarkable_version variable is used in two different contexts:

  1. For UpdateManager method calls (lines 64, 66, 85) - these expect HardwareType enum
  2. For comparisons (lines 73, 75, 77) - these compare against HardwareType enum values

But I also see that later in the code (around line 205-217), there's a remarkable.hardware property that's used consistently as a HardwareType enum.

The cast to str appears to be incorrect. The variable should remain as HardwareType enum throughout. If there are places where a string representation is actually needed, those should use .value or .name on the enum instead of casting the enum to string.

Could you point me to the specific lines where remarkable_version is used as a string? I may have missed them in my analysis, and it would help determine the best approach for fixing this type inconsistency.

✏️ Learnings added 
Learnt from: Eeems
PR: Jayy001/codexctl#140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.136Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


version = cast(str | None, args.get("version", None))
version = cast(Callable[[str, None], str | None], args.get)("version", None)

if remarkable_version:
if version == "latest":
Expand Down Expand Up @@ -177,7 +174,8 @@ def call_func(self, function: str, args: dict) -> None:
)
else:
rmWeb.upload(input_paths=args["paths"], remoteFolder=args["remote"])



### Update & Version functionalities
elif function in ("install", "status", "restore"):
remote = False
Expand All @@ -198,9 +196,9 @@ def call_func(self, function: str, args: dict) -> None:

remarkable = DeviceManager(
remote=remote,
address=args["address"],
address=cast(str, args["address"]),
logger=self.logger,
authentication=args["password"],
authentication=cast(str, args["password"]),
)

if version == "latest":
Expand Down Expand Up @@ -285,7 +283,7 @@ def version_lookup(version: str | None) -> re.Match[str] | None:
) != os.path.abspath("updates"):
if not os.path.exists("updates"):
os.mkdir("updates")
shutil.move(update_file, "updates")
_ = shutil.move(update_file, "updates")
update_file = get_available_version(version)
made_update_folder = True # Delete at end

Expand Down Expand Up @@ -323,7 +321,7 @@ def version_lookup(version: str | None) -> re.Match[str] | None:
remarkable.install_ohma_update(update_file)

if made_update_folder: # Move update file back out
shutil.move(os.listdir("updates")[0], "../")
_ = shutil.move(os.listdir("updates")[0], "../")
shutil.rmtree("updates")

os.chdir(orig_cwd)
Expand All @@ -337,23 +335,23 @@ def main() -> None:

### Setting up the argument parser
parser = argparse.ArgumentParser("Codexctl")
parser.add_argument(
_ = parser.add_argument(
"--verbose",
"-v",
required=False,
help="Enable verbose logging",
action="store_true",
dest="verbose",
)
parser.add_argument(
_ = parser.add_argument(
"--address",
"-a",
required=False,
help="Specify the address of the device",
default=None,
dest="address",
)
parser.add_argument(
_ = parser.add_argument(
"--password",
"-p",
required=False,
Expand All @@ -374,9 +372,9 @@ def main() -> None:
download = subparsers.add_parser(
"download", help="Download the specified version firmware file"
)
download.add_argument("version", help="Version to download")
download.add_argument("--out", "-o", help="Folder to download to", default=None)
download.add_argument(
_ = download.add_argument("version", help="Version to download")
_ = download.add_argument("--out", "-o", help="Folder to download to", default=None)
_ = download.add_argument(
"--hardware",
"--device",
"-d",
Expand All @@ -389,35 +387,35 @@ def main() -> None:
backup = subparsers.add_parser(
"backup", help="Download remote files to local directory"
)
backup.add_argument(
_ = backup.add_argument(
"-r",
"--remote",
help="Remote directory to backup. Defaults to download folder",
default="",
dest="remote",
)
backup.add_argument(
_ = backup.add_argument(
"-l",
"--local",
help="Local directory to backup to. Defaults to download folder",
default="./",
dest="local",
)
backup.add_argument(
_ = backup.add_argument(
"-R",
"--no-recursion",
help="Disables recursively backup remote directory",
action="store_true",
dest="no_recursion",
)
backup.add_argument(
_ = backup.add_argument(
"-O",
"--no-overwrite",
help="Disables overwrite",
action="store_true",
dest="no_overwrite",
)
backup.add_argument(
_ = backup.add_argument(
"-i",
"--incremental",
help="Overwrite out-of-date files only",
Expand All @@ -428,40 +426,48 @@ def main() -> None:
cat = subparsers.add_parser(
"cat", help="Cat the contents of a file inside a firmwareimage"
)
cat.add_argument("file", help="Path to update file to cat", default=None)
cat.add_argument("target_path", help="Path inside the image to list", default=None)
_ = cat.add_argument("file", help="Path to update file to cat", default=None)
_ = cat.add_argument(
"target_path", help="Path inside the image to list", default=None
)

### Ls subcommand
ls = subparsers.add_parser("ls", help="List files inside a firmware image")
ls.add_argument("file", help="Path to update file to extract", default=None)
ls.add_argument("target_path", help="Path inside the image to list", default=None)
_ = ls.add_argument("file", help="Path to update file to extract", default=None)
_ = ls.add_argument(
"target_path", help="Path inside the image to list", default=None
)

### Extract subcommand
extract = subparsers.add_parser(
"extract", help="Extract the specified version update file"
)
extract.add_argument("file", help="Path to update file to extract", default=None)
extract.add_argument("--out", help="Folder to extract to", default=None, dest="out")
_ = extract.add_argument(
"file", help="Path to update file to extract", default=None
)
_ = extract.add_argument(
"--out", help="Folder to extract to", default=None, dest="out"
)

### Mount subcommand
mount = subparsers.add_parser(
"mount", help="Mount the specified version firmware filesystem"
)
mount.add_argument(
_ = mount.add_argument(
"filesystem",
help="Path to version firmware filesystem to extract",
default=None,
)
mount.add_argument("--out", help="Folder to mount to", default=None)
_ = mount.add_argument("--out", help="Folder to mount to", default=None)

### Upload subcommand
upload = subparsers.add_parser(
"upload", help="Upload folder/files to device (pdf only)"
)
upload.add_argument(
_ = upload.add_argument(
"paths", help="Path to file(s)/folder to upload", default=None, nargs="+"
)
upload.add_argument(
_ = upload.add_argument(
"-r",
"--remote",
help="Remote directory to upload to. Defaults to root folder",
Expand All @@ -470,12 +476,12 @@ def main() -> None:
)

### Status subcommand
subparsers.add_parser(
_ = subparsers.add_parser(
"status", help="Get the current version of the device and other information"
)

### Restore subcommand
subparsers.add_parser(
_ = subparsers.add_parser(
"restore", help="Restores to previous version installed on device"
)

Expand Down
4 changes: 2 additions & 2 deletions codexctl/analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import ext4
import warnings
import warnings
import errno

from remarkable_update_image import UpdateImage
from remarkable_update_image import UpdateImageSignatureException


def get_update_image(file: str):
def get_update_image(file: str) -> tuple[UpdateImage, ext4.Volume]:
"""Extracts files from an update image (<3.11 currently)"""

image = UpdateImage(file)
Expand Down
Loading
Loading