Skip to content

Refactor filesystem_size_quartet method #1228

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

Merged
mulkieran merged 3 commits into from
Sep 30, 2025
Merged

Refactor filesystem_size_quartet method #1228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90c8678
Add type annotations for _formatting module
mulkieran Sep 30, 2025
5ce038d
Make type of size param reflect true usage
mulkieran Sep 30, 2025
77f5e2c
Refactor filesystem_size_quartet
mulkieran Sep 30, 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
30 changes: 22 additions & 8 deletions src/stratis_cli/_actions/_formatting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@

# isort: STDLIB
import sys
from typing import Any, Callable, List, Optional
from uuid import UUID

# isort: THIRDPARTY
from dbus import Struct
from justbytes import Range
from wcwidth import wcswidth

# placeholder for tables where a desired value was not obtained from stratisd
Expand All @@ -33,7 +36,7 @@
TOTAL_USED_FREE = "Total / Used / Free"


def size_triple(size, used):
def size_triple(size: Range, used: Optional[Range]) -> str:
"""
Given size and used, return a properly formatted string Total/ Used / Free

Expand All @@ -44,16 +47,16 @@ def size_triple(size, used):
:rtype: str
:returns: formatted string for display
"""
free = None if size is None or used is None else size - used
free = None if used is None else size - used

return (
f"{TABLE_FAILURE_STRING if size is None else size} / "
f"{size} / "
f"{TABLE_FAILURE_STRING if used is None else used} / "
f"{TABLE_FAILURE_STRING if free is None else free}"
)


def get_property(prop, to_repr, default):
def get_property(prop: Struct, to_repr: Callable, default: Optional[Any]):
"""
Get a representation of an optional D-Bus property. An optional
D-Bus property is one that may be unknown to stratisd.
Expand All @@ -70,7 +73,7 @@ def get_property(prop, to_repr, default):
return to_repr(value) if valid else default


def _get_column_len(column_width, entry_len, entry_width):
def _get_column_len(column_width: int, entry_len: int, entry_width: int) -> int:
"""
From the desired column width in cells and the item to be printed,
calculate the required number of characters to pass to the format method.
Expand All @@ -94,7 +97,13 @@ def _get_column_len(column_width, entry_len, entry_width):
return column_width - (entry_width - entry_len)


def _print_row(file, row, row_widths, column_widths, column_alignments):
def _print_row(
file: Any,
row: Any,
row_widths: List[int],
column_widths: List[int],
column_alignments: List[str],
):
"""
Print a single row in a table. The row might be the header row, or
a row of data items.
Expand All @@ -117,7 +126,12 @@ def _print_row(file, row, row_widths, column_widths, column_alignments):
print(" ".join(entries), end="", file=file)


def print_table(column_headings, row_entries, alignment, file=sys.stdout):
def print_table(
column_headings: List[str],
row_entries: List[Any],
alignment: List[str],
file=sys.stdout,
):
"""
Given the column headings and the row_entries, print a table.
Align according to alignment specification and always pad with 2 spaces.
Expand Down Expand Up @@ -156,7 +170,7 @@ def print_table(column_headings, row_entries, alignment, file=sys.stdout):
print(file=file)


def get_uuid_formatter(unhyphenated):
def get_uuid_formatter(unhyphenated: bool) -> Callable:
"""
Get a function to format UUIDs.

Expand Down
18 changes: 9 additions & 9 deletions src/stratis_cli/_actions/_list_filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

# isort: STDLIB
from abc import ABC, abstractmethod
from typing import Any, Callable, Dict, List
from typing import Any, Callable, Dict, List, Optional

# isort: THIRDPARTY
from dateutil import parser as date_parser
Expand Down Expand Up @@ -124,26 +124,26 @@ def display(self):
List the filesystems.
"""

def filesystem_size_quartet(dbus_props):
def filesystem_size_quartet(
total: Range, used: Optional[Range], limit: Optional[Range]
) -> str:
"""
Calculate the triple to display for filesystem size.

:param dbus_props: filesystem D-Bus properties
:type dbus_props: MOFilesystem

:returns: a string a formatted string showing all three values
:rtype: str
"""
total = Range(dbus_props.Size())
used = get_property(dbus_props.Used(), Range, None)
limit = get_property(dbus_props.SizeLimit(), Range, None)
return f'{size_triple(total, used)} / {"None" if limit is None else limit}'

tables = [
(
self.pool_object_path_to_pool_name[mofilesystem.Pool()],
mofilesystem.Name(),
filesystem_size_quartet(mofilesystem),
filesystem_size_quartet(
Range(mofilesystem.Size()),
get_property(mofilesystem.Used(), Range, None),
get_property(mofilesystem.SizeLimit(), Range, None),
),
Comment on lines +142 to +146
Copy link

@coderabbitai coderabbitai bot Sep 30, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify that mofilesystem.Size() returns a value compatible with Range constructor.

The refactored call site now explicitly constructs Range(mofilesystem.Size()) instead of relying on implicit conversions. Ensure that mofilesystem.Size() returns a numeric value that the Range constructor accepts (likely an integer representing bytes).

🏁 Script executed:

#!/bin/bash
# Description: Verify the type signature of mofilesystem.Size() and its compatibility with Range constructor

# Search for MOFilesystem class and Size method definition
ast-grep --pattern $'class MOFilesystem {
  $$$
  Size($$$) {
    $$$
  }
  $$$
}'

# Also search for Size method calls to understand return type
rg -nP --type=py -A3 'def Size\('

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Locate MOFilesystem class definition
rg -nP 'class\s+MOFilesystem' -C3 -g '*.py'
# Locate any Size method definitions
rg -nP 'def\s+Size\(' -C3 -g '*.py'
# Locate Range class constructor
rg -nP 'class\s+Range' -C3 -g '*.py'
# Print top of list_filesystem action file for imports context
sed -n '1,50p' src/stratis_cli/_actions/_list_filesystem.py

Length of output: 1606

🏁 Script executed:

#!/bin/bash
# Print first part of the data module to locate MOFilesystem and its Size method
sed -n '1,200p' src/stratis_cli/_actions/_data.py
# Specifically search for the Size method in the data module
rg -nP 'def\s+Size\(' -C2 -g 'src/stratis_cli/_actions/_data.py'

# Inspect justbytes.Range constructor signature if installed
python3 - << 'EOF'
import inspect
try:
    from justbytes import Range
    print("Range signature:", inspect.signature(Range))
except Exception as e:
    print("Error importing or inspecting Range:", e)
EOF

Length of output: 5491

🏁 Script executed:

#!/bin/bash
# Locate interface spec for Filesystem in introspect module
grep -R 'org.storage.stratis3.Filesystem' -n src/stratis_cli/_actions/_introspect.py
# Print context around the Filesystem interface definition
sed -n '100,300p' src/stratis_cli/_actions/_introspect.py

Length of output: 8492

Convert MOFilesystem.Size() to int before passing to Range. The D-Bus Size property is defined as a string (type="s"), so MOFilesystem.Size() returns a str. Use Range(int(mofilesystem.Size())) to ensure the constructor receives a numeric value.

🤖 Prompt for AI Agents 
In src/stratis_cli/_actions/_list_filesystem.py around lines 142 to 146, the
call passes MOFilesystem.Size() (a D-Bus string) into Range which expects a
numeric value; convert it to an int before constructing Range by calling
Range(int(mofilesystem.Size())), keeping the other get_property calls unchanged
so the filesystem_size_quartet receives a numeric Size.

mofilesystem.Devnode(),
self.uuid_formatter(mofilesystem.Uuid()),
)
Expand Down
Loading