`util.py` module type hints by laserkelvin · Pull Request #192 · P3HPC/code-base-investigator

laserkelvin · 2025-05-08T17:10:24Z

Related issues

Closes #191, which is part of epic #182.

Proposed changes

This PR makes (currently) purely aesthetic changes by adding type hints to function signatures contained in util.py, as well as updating docstrings to match implementations.

Bumps copyright year at the beginning of the module
Adds type annotations for safe_open_write_binary, valid_path
Updated type annotations and docstrings for ensure_ext, _load_toml

laserkelvin · 2025-05-08T17:14:00Z

My last commit has the DCO sign-off - not sure why it's not passing?

Copilot

Pull Request Overview

This PR adds type hints and updates docstrings in the util.py module for improved code clarity and consistency. Key changes include updating the copyright year, adding type annotations for safe_open_write_binary and valid_path, and refining the _load_toml return type and documentation.

Comments suppressed due to low confidence (1)

codebasin/util.py:55

The extra 'f' in the formatted string appears to be a mistake. It should be removed so that the error message is formatted using the variable 'exts' correctly.

raise ValueError(f"{path} does not have a valid extension: f{exts}")

Copilot · 2025-05-09T08:04:14Z

codebasin/util.py



-def safe_open_write_binary(fname):
+def safe_open_write_binary(fname: os.PathLike[str]) -> TextIOWrapper:


The return type annotation 'TextIOWrapper' is inconsistent with opening a file in binary write mode. Consider changing it to a type such as 'BinaryIO' or another appropriate binary stream type.

Suggested change

def safe_open_write_binary(fname: os.PathLike[str]) -> TextIOWrapper:

def safe_open_write_binary(fname: os.PathLike[str]) -> typing.BinaryIO:

I don't know enough about the differences here. Could you try making the change and see if the type-hinting tools are still satisfied?

LSP seems happy with change made in 8e293ed

Pennycook · 2025-05-09T08:06:07Z

My last commit has the DCO sign-off - not sure why it's not passing?

Because we don't squash on merge, all commits in the PR need the DCO sign-off.

I think the only way to add the missing sign-off retroactively is to do an interactive rebase, add the missing sign-offs, then force push.

Pennycook · 2025-05-09T08:28:26Z

codebasin/util.py



-def ensure_ext(path: os.PathLike[str], extensions: Iterable[str]):
+def ensure_ext(path: os.PathLike[str], extensions: Iterable[str]) -> None:


I'm not opposed to this, but I'm curious. Do conventions say to specify -> None in the case of no return types? I'm not sure which style guides to look at here.

My thought there was that Python functions always return None, even if they don't have a return statement. For the developer it seems to be better to be explicit, since then you remove the ambiguity of "did we leave out the return signature, or does it really return None"?

Seems like mypy recommends this as well, even if we don't use mypy.

Okay, that's good enough for me!

Pennycook · 2025-05-09T08:29:03Z

codebasin/util.py

@@ -59,7 +55,7 @@ def ensure_ext(path: os.PathLike[str], extensions: Iterable[str]):
        raise ValueError(f"{path} does not have a valid extension: f{exts}")


You didn't change this, but I think Copilot is right. There shouldn't be a second "f" here, right?

Patched in 55af105

Pennycook · 2025-05-09T08:32:51Z

codebasin/util.py



-def safe_open_write_binary(fname):
+def safe_open_write_binary(fname: os.PathLike[str]) -> TextIOWrapper:


I don't know enough about the differences here. Could you try making the change and see if the type-hinting tools are still satisfied?

Pennycook · 2025-05-09T08:33:43Z

codebasin/util.py

+    This function ensures that the file path does not contain
+    potentially dangerous characters such as null bytes (`\x00`)
+    or carriage returns/line feeds (`\n`, `\r`). These characters
+    can pose security risks, particularly in file handling operations.


Suggested change

This function ensures that the file path does not contain

potentially dangerous characters such as null bytes (`\x00`)

or carriage returns/line feeds (`\n`, `\r`). These characters

can pose security risks, particularly in file handling operations.

This function ensures that the file path does not contain

potentially dangerous characters such as null bytes (`\x00`)

or carriage returns/line feeds (`\n`, `\r`).

Removed in c7ef2eb

Pennycook · 2025-05-09T08:34:23Z

codebasin/util.py

+    Notes
+    -----
+    - This function is useful for validating file paths before performing
+      file I/O operations to prevent security vulnerabilities.
+


I think we should remove this.

Suggested change

Notes

-----

- This function is useful for validating file paths before performing

file I/O operations to prevent security vulnerabilities.

Removed in c7ef2eb

Pennycook · 2025-05-09T08:47:34Z

codebasin/util.py

+    dict[str, Any]
+        The loaded TOML object, represented as a Python
+        dict with str key/value mappings.


I haven't thought about this before, so want to double-check you agree.

I think this was set up as -> object before because when we were loading JSON, the result was not guaranteed to be a dict. For example, a compilation database is an array of objects.

It does seem like TOML must be key-value pairs, and the documentation says that "TOML is designed to map unambiguously to a hash table". So I think this is right...?

Yup, that's why I left the JSON signature as object, but updated it for the TOML. tomllib.load's signature only returns dict[str, Any], because I think by design it can't return anything else (i.e. keys have to be strings).

Pennycook · 2025-05-09T14:55:27Z

Thanks, @laserkelvin. All of these changes look good to me, assuming you fix the DCO sign-off.

I'm not going to formally approve it yet, because I don't want to accidentally merge it before the next release.

Function does not actually return `bool` Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

`tomllib.load` signature returns a dict; this change matches what is ultimately returned by `tomllib.load`. Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

laserkelvin requested a review from Pennycook May 8, 2025 17:10

laserkelvin added the documentation Improvements or additions to documentation label May 8, 2025

laserkelvin force-pushed the util-type-annotations branch from 5b26279 to 12e23ad Compare May 8, 2025 17:11

Pennycook requested a review from Copilot May 9, 2025 08:03

Copilot AI reviewed May 9, 2025

View reviewed changes

Pennycook reviewed May 9, 2025

View reviewed changes

Kin Long Kelvin Lee added 9 commits May 9, 2025 07:55

docs: correcting docstring and updating return signature

3e8852c

Function does not actually return `bool` Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

docs: adding type annotations to save_open_write_binary

d5e36b3

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

docs: added docstring and type annotations to valid_path

fdb7113

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

chore: bumping copyright year

ad6579c

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

docs: making _load_toml type hint specific

29e5530

`tomllib.load` signature returns a dict; this change matches what is ultimately returned by `tomllib.load`. Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

style: ran pre-commit workflows on module

72b4e9b

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

refactor: replacing TextIOWrapper typehint in favor of BinaryIO

77e47a1

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

fix: removing redundant f in f-string

f340291

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

docs: removing unnecessary commentary from docstrings

dca4807

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>

laserkelvin force-pushed the util-type-annotations branch from c7ef2eb to dca4807 Compare May 9, 2025 14:56

Pennycook approved these changes May 20, 2025

View reviewed changes

Pennycook merged commit f52f376 into P3HPC:main May 20, 2025
4 checks passed



		def safe_open_write_binary(fname):
		def safe_open_write_binary(fname: os.PathLike[str]) -> TextIOWrapper:



		def ensure_ext(path: os.PathLike[str], extensions: Iterable[str]):
		def ensure_ext(path: os.PathLike[str], extensions: Iterable[str]) -> None:

		@@ -59,7 +55,7 @@ def ensure_ext(path: os.PathLike[str], extensions: Iterable[str]):
		raise ValueError(f"{path} does not have a valid extension: f{exts}")

Conversation

laserkelvin commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed changes

Uh oh!

laserkelvin commented May 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pennycook commented May 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pennycook commented May 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

laserkelvin commented May 8, 2025 •

edited

Loading