New column types + example columns

[joce]

Summary

Adds support for new column types in the quote table:

• Enums (multiple types)
• Date
• Datetime
• Bool

Changes

• Extended formatting utilities for new data types
• Added column definitions with new type support
• Included comprehensive tests for formatting and table cells
• Minor yquote module update

[joce]

@codex review

[claude]

Claude finished @joce's task in 1m 25s —— View job

---

Code Review Complete ✓

Reviewing PR #30: New column types + example columns

Todo List

• Read all changed files
• Review code quality and adherence to AGENTS.md guidelines
• Check security considerations
• Evaluate performance implications
• Verify test coverage and quality
• Review documentation completeness
• Post comprehensive review feedback

---

Review Summary

This PR adds strong support for new column types (Date, DateTime, Enum, Boolean) with excellent test coverage and clean implementation. The code is well-structured, follows project conventions, and includes comprehensive tests. I found a few minor issues that should be addressed.

Code Quality: Excellent ✓

Strengths

• Clean, consistent implementation across all new cell types
• Proper use of type hints and from __future__ import annotations
• Good separation of concerns between formatting and cell rendering
• Follows Google-style docstrings consistently
• Consistent error handling (None values treated as -inf for sorting)

Issues Found

1. Inconsistent None Handling in EnumCell (src/appui/quote_column_definitions.py:284)

Severity: Minor

The EnumCell class handles None differently than other cell types - it uses an empty string for the sort key instead of a tuple with a sentinel value:

primary = display_value.lower() if value is not None else ""
sort_key = (primary, secondary_key.lower()) if secondary_key else (primary,)

All other cell types use -inf as the sentinel. This inconsistency could lead to unexpected sorting behavior.

Recommendation: Use a consistent pattern like float("-inf") for the primary sort key when value is None, or document why strings work differently.

2. TextCell Sort Key Construction Duplication (src/appui/quote_column_definitions.py:76-83)

Severity: Minor

The sort key construction logic is duplicated within the TextCell constructor:

primary = value if case_sensitive else value.lower()
if secondary_key:
    sort_key = (
        primary,
        secondary_key if case_sensitive else secondary_key.lower(),
    )
else:
    sort_key = (primary,)

This could be simplified using the existing _with_secondary_key helper or by creating a similar helper for text-based cells.

Recommendation: Refactor to reduce duplication, perhaps:

primary = value if case_sensitive else value.lower()
secondary = (secondary_key if case_sensitive else secondary_key.lower()) if secondary_key else None
sort_key = (primary, secondary) if secondary else (primary,)

3. Test Duplication in test_as_compact_int (tests/appui/test_formatting.py:69-70)

Severity: Trivial

Line 70 duplicates line 69:

assert fmt.as_compact(input_value) == expected_output
assert fmt.as_compact(input_value) == expected_output

Recommendation: Remove the duplicate line.

Security: Good ✓

No security concerns identified. The code:

• Properly validates input types through type hints and Pydantic
• Uses safe string formatting (f-strings, strftime)
• No user input is executed or eval'd
• No external resources accessed

Performance: Good ✓

Strengths

• Efficient sorting using numeric/ordinal keys
• No O(n²) operations
• Proper use of constants for repeated values

Observations

• Date/DateTime conversion to ordinal/timestamp for sorting is efficient
• String lowercasing for case-insensitive sorting is appropriate for the use case
• The _get_style_for_value function is simple and fast

No performance concerns identified.

Testing: Excellent ✓

Strengths

• Comprehensive parametrized tests covering edge cases
• Tests for None handling across all formatters
• Good test organization and naming
• Proper use of pytest markers (would benefit from seeing @pytest.mark.ui if applicable)
• All test docstrings are concise and clear

Coverage Analysis

Based on the code changes:

• ✅ All formatting functions tested (as_percent, as_float, as_compact, as_date, as_datetime, as_enum, as_bool)
• ✅ All cell types tested (TextCell, TickerCell, FloatCell, PercentCell, CompactNumberCell, DateCell, DateTimeCell, EnumCell, BooleanCell)
• ✅ None handling tested for all types
• ✅ Sorting behavior tested
• ✅ Format override tested for date/datetime
• ✅ Default justification tested for all cell types

Minor Gap

The _as_title_case private helper function in formatting.py:20-30 is not directly tested. While it's covered through as_enum tests, explicit tests for edge cases would be beneficial:

• Multiple consecutive underscores
• Leading/trailing underscores
• Already title-cased input

Recommendation: Add tests for _as_title_case edge cases, or document that it's sufficiently covered through as_enum.

Documentation: Excellent ✓

Strengths

• All public functions have complete Google-style docstrings
• All class constructors are well-documented
• Type hints are comprehensive and correct
• TODO comment in formatting.py:17 is clear and actionable
• Docstrings match actual behavior

Observations

• The yquote.py change (line 710) adds a tradeable field with proper documentation
• All new cell types have clear docstrings
• Parameter documentation is thorough

No documentation issues identified.

Additional Observations

1. Positive Patterns

• The _with_secondary_key helper is a nice abstraction (src/appui/quote_column_definitions.py:34-50)
• Consistent use of _NO_VALUE constant across formatters
• Good choice of Unicode checkbox characters (☑/☐)
• The format override parameters for date/datetime provide good flexibility

2. Future Considerations

The TODO comment about user-configurable date/time formats is noted. When implementing this, consider:

• How format preferences will be passed to cell factories
• Whether format validation is needed
• Impact on existing column definitions

3. Minor Yquote Change

The change to yquote.py (line 710) changes tradeable from optional to required:

tradeable: bool

This is fine if all Yahoo Finance quotes include this field, but verify that this won't break parsing for any quote types.

Recommendations Summary

Must Fix

1. Remove duplicate assertion in test_formatting.py:70

Should Fix

2. Standardize None handling in EnumCell to match other cell types
3. Simplify TextCell sort key construction to reduce duplication

Nice to Have

4. Add explicit tests for _as_title_case edge cases
5. Verify tradeable field is always present in Yahoo Finance responses

Overall Assessment

This is high-quality code that follows project conventions excellently. The implementation is clean, well-tested, and properly documented. The issues identified are all minor and don't block merging, though fixing the duplicate assertion and standardizing the EnumCell behavior would improve consistency.

Recommendation: Approve with minor changes suggested

---

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @joce's task in 2m 12s —— View job

---

Code Review Complete ✓

Reviewing PR #30: New column types + example columns

---

Review Summary

This PR successfully adds support for new column types (Date, DateTime, Enum, Boolean) with excellent implementation quality. The author has addressed all previous review comments, and the code now demonstrates strong adherence to project guidelines with comprehensive test coverage.

Code Quality: Excellent ✓

Strengths

• Clean, consistent implementation across all new cell types
• Proper use of type hints and from __future__ import annotations
• Good separation of concerns between formatting and cell rendering
• Follows Google-style docstrings consistently per AGENTS.md
• Previous review issues resolved: Duplicate assertion removed, TextCell refactored, _as_title_case tests added
• Consistent error handling (None values treated as -inf for sorting)
• Elegant use of helper functions (_with_secondary_key)

Issues Found

1. Inconsistent None Handling in EnumCell (src/appui/quote_column_definitions.py:283-284)

Severity: Minor

The EnumCell class handles None values differently from all other cell types. It uses an empty string "" as the sentinel for the primary sort key, while all other cells use float("-inf"):

# EnumCell (line 283-284)
primary = display_value.lower() if value is not None else ""
sort_key = (primary, secondary_key.lower()) if secondary_key else (primary,)

Compare with other cells:

# FloatCell, PercentCell, CompactNumberCell, DateCell, DateTimeCell, BooleanCell
safe_value = float("-inf") if value is None else ...

Impact: When sorting a column containing enum cells, None values will sort differently than in other column types. String "" sorts differently than float("-inf") when mixed in tuples, which could lead to unexpected behavior if secondary keys are used.

Recommendation: Standardize to use float("-inf") for consistency:

primary = display_value.lower() if value is not None else float("-inf")

Fix this →

2. Missing Secondary Key Lower-Casing in EnumCell (src/appui/quote_column_definitions.py:284)

Severity: Minor

The EnumCell unconditionally lowercases the secondary_key when building the sort key, but doesn't check if secondary_key is None first:

sort_key = (primary, secondary_key.lower()) if secondary_key else (primary,)

While this works because of the conditional check, it's inconsistent with the refactored TextCell pattern (lines 77-82) which explicitly handles the None case.

Recommendation: For consistency with TextCell, explicitly handle the None case:

secondary = secondary_key.lower() if secondary_key else None
sort_key = (primary, secondary) if secondary else (primary,)

Security: Excellent ✓

No security concerns identified. The code:

• Properly validates input types through type hints and Pydantic
• Uses safe string formatting (f-strings, strftime)
• No user input is executed or eval'd
• No external resources accessed
• No injection vulnerabilities

Performance: Excellent ✓

Strengths

• Efficient sorting using numeric/ordinal keys (dates use toordinal(), datetimes use timestamp())
• No O(n²) operations - all formatting is O(1)
• Proper use of constants for repeated values
• String lowercasing for case-insensitive sorting is appropriate for TUI use case

Observations

• The _with_secondary_key helper avoids tuple construction overhead when no secondary key exists
• All cell constructors pre-compute sort keys, avoiding repeated computation during table operations

No performance concerns identified. Well-optimized for real-time TUI updates per AGENTS.md requirements.

Testing: Excellent ✓

Strengths

• Comprehensive parametrized tests covering edge cases per AGENTS.md guidelines
• Previous review gap addressed: _as_title_case now has extensive edge case tests (lines 128-161)
• Tests for None handling across all formatters
• Good test organization and naming
• All test docstrings are concise and clear per AGENTS.md style
• Proper use of pytest.mark.parametrize for multiple inputs
• All branches covered

Coverage Analysis

✅ All formatting functions tested (as_percent, as_float, as_compact, as_date, as_datetime, as_enum, as_bool)
✅ All cell types tested (TextCell, TickerCell, FloatCell, PercentCell, CompactNumberCell, DateCell, DateTimeCell, EnumCell, BooleanCell)
✅ None handling tested for all types
✅ Sorting behavior tested
✅ Format override tested for date/datetime
✅ Default justification tested for all cell types
✅ _as_title_case edge cases tested (multiple underscores, leading/trailing, mixed case)

Test Quality Observations

• Tests follow AGENTS.md guidelines: clear naming, short docstrings, no parameter documentation
• Good use of pytest IDs for test identification
• # pyright: reportPrivateUsage=none appropriately used for testing private helpers

Documentation: Excellent ✓

Strengths

• All public functions have complete Google-style docstrings per AGENTS.md
• All class constructors are well-documented
• Type hints are comprehensive and correct
• Docstrings use imperative mood ("Initialize", "Return") per AGENTS.md
• White line between docstring and code per AGENTS.md
• TODO comment (formatting.py:17) is clear and actionable

Documentation Quality

• Parameter and return type documentation is thorough
• The yquote.py change (line 711) properly documents the tradeable field
• All new cell types have clear docstrings explaining their purpose

Additional Observations

1. Positive Patterns ⭐

• The refactored TextCell (lines 76-82) now matches the recommended pattern from the previous review
• Consistent use of _NO_VALUE constant across formatters
• Excellent choice of Unicode checkbox characters (☑/☐) for boolean rendering
• Format override parameters provide good flexibility for future user configuration

2. Code Style Compliance

✅ Follows Google Python Style Guide
✅ Uses from __future__ import annotations
✅ Type hints on all functions and methods
✅ Uses | syntax for unions
✅ Private helpers prefixed with _
✅ No wildcard imports
✅ f-strings used appropriately
✅ No mutable default arguments

3. Yquote Change

The change to yquote.py (line 711) changes tradeable from optional to required:

tradeable: bool

This is appropriate if Yahoo Finance consistently provides this field. The new BooleanCell and column definition (quote_column_definitions.py:581-594) properly handle this field.

4. Previous Review Items - Status

From the previous Claude review:

1. ✅ FIXED: Duplicate assertion removed (test_formatting.py:70)
2. ✅ FIXED: TextCell refactored to reduce duplication (quote_column_definitions.py:76-82)
3. ⚠️ PARTIALLY ADDRESSED: EnumCell None handling still uses empty string instead of -inf
4. ✅ FIXED: _as_title_case edge cases now tested (test_formatting.py:128-161)

Recommendations Summary

Should Fix (Non-Blocking)

1. Standardize EnumCell None handling to use float("-inf") instead of "" for consistency with other cell types (quote_column_definitions.py:283)
2. Refactor EnumCell secondary key handling to match TextCell pattern for consistency (quote_column_definitions.py:284)

Nice to Have

• Consider documenting the sort key design pattern (using -inf for None values) in a module-level comment, as it's a critical pattern used across all cell types

Overall Assessment

This is excellent code that demonstrates strong software engineering practices and full adherence to the AGENTS.md guidelines. The implementation is:

• Clean and maintainable
• Well-tested with comprehensive coverage
• Properly documented
• Performance-conscious
• Secure

The author has done an excellent job addressing previous review feedback. The remaining issues are minor consistency improvements that would enhance code uniformity but do not block merging.

Recommendation: Approve - Minor improvements suggested for consistency

---