Skip to content

Feature/all columns#32

Merged
joce merged 2 commits intomasterfrom
feature/all-columns
Jan 11, 2026
Merged

Feature/all columns#32
joce merged 2 commits intomasterfrom
feature/all-columns

Conversation

@joce
Copy link
Owner

@joce joce commented Jan 11, 2026

Summary

  • Add 70+ new column definitions covering all YQuote fields (moving averages, bid/ask, earnings, dividends, ETF/crypto-specific data, pre/post market prices, etc.)
  • Display full column names in the column chooser for better discoverability

Test plan

  • Verify column chooser displays full names instead of short labels
  • Confirm new columns render correctly when added to the Watchlist

@claude
Copy link

claude bot commented Jan 11, 2026

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


Code Review Completed

Tasks

  • Read and analyze all changed files
  • Review quote_column_definitions.py (350+ lines of new columns)
  • Review column_chooser_screen.py changes
  • Review test updates
  • Review CSS and spell check changes
  • Provide comprehensive feedback

Critical Issues Found ⚠️

35 Missing Width Parameters in ColumnSpec Definitions

The most critical issue is that 35 of the new ColumnSpec definitions are missing the required width parameter (4th positional argument). This will cause runtime errors when these columns are used.

Impact: These columns will crash the application when selected.

I've added inline comments on each problematic line with the corrected definition. All missing width parameters need to be added before this PR can be merged.

Example of the issue:

# ❌ WRONG - Missing width parameter
ColumnSpec(
    "day_range",
    "Day Range", 
    "Day Range",
    attr_name="regular_market_day_range",
)

# ✅ CORRECT - Width parameter (20) included
ColumnSpec(
    "day_range",
    "Day Range",
    "Day Range", 
    20,  # <-- This is required!
    attr_name="regular_market_day_range",
)

Positive Aspects ✓

  1. Excellent Coverage: Adding 70+ columns covering all YQuote fields is comprehensive and valuable
  2. Good Organization: Columns are logically grouped by category (Identity, Price Data, Moving Averages, etc.)
  3. Consistent Naming: Full names follow a consistent pattern and are descriptive
  4. Proper Documentation: The docstring at the end documenting excluded fields is excellent
  5. Test Updates: Tests were properly updated to reflect the full_name changes
  6. UI Improvement: Using full names in the column chooser significantly improves discoverability

Code Quality Review

Column Chooser Changes (column_chooser_screen.py)

Status: ✅ Good

  • Changes are minimal and focused (3 lines changed)
  • Properly uses full_name instead of label for better UX
  • Click selector fix ("ListView ListItem Label") is correct

Test Updates (test_column_chooser_screen.py)

Status: ✅ Good

  • Tests properly updated to use full_name
  • Good test coverage for the new behavior
  • Consistent with testing standards

CSS Changes (doubloon_app.tcss)

Status: ✅ Good

  • New frozen label styling is appropriate
  • Consistent with existing patterns
  • Proper focus state handling

Spell Check Dictionary (.cspell.json)

Status: ✅ Good

  • Added "triggerable" to dictionary (references existing YQuote field)

Additional Observations

Type Safety & Consistency

The new columns follow the existing patterns well:

  • Proper use of PercentCell for percentage values
  • Proper use of style_fn=_get_style_for_value for gain/loss coloring
  • Appropriate attr_name mappings for YQuote fields

Code Style Compliance

  • Follows Google Python Style Guide ✓
  • Proper type hints would be inferred from ColumnSpec dataclass ✓
  • Docstrings follow project conventions ✓
  • Python 3.10+ compatibility maintained ✓

Performance Considerations

Minor concern: Adding 70+ columns increases the data structure size, but this is acceptable given:

  • Columns are lazy-loaded via dictionary lookup
  • Users only activate the columns they need
  • The dictionary comprehension in ALL_QUOTE_COLUMNS is efficient

Recommendations

Critical (Must Fix)

  1. Add missing width parameters to all 35 ColumnSpecs I've marked in inline comments

Best Practice (Should Consider)

  1. After fixing, run tox -e py3.10 to verify basic functionality
  2. Run full tox and make spell before final merge
  3. Consider adding a unit test to validate all ColumnSpecs have required parameters

Summary

This PR adds significant value by exposing all YQuote fields as columns, but it cannot be merged until the 35 missing width parameters are fixed. Once corrected, this will be a solid enhancement.

The column chooser UI improvement (showing full names) is well-implemented and the test coverage is good.

Required Action: Fix all 35 missing width parameters identified in the inline comments.


@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' 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!

attr_name="two_hundred_day_average",
),
ColumnSpec(
"_200d_avg_change",
Copy link

Choose a reason for hiding this comment

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

Missing width parameter: Missing width parameter.

Expected: ColumnSpec("_200d_avg_change", "200d Chg", "200-Day Average Change", 10, attr_name="two_hundred_day_average_change", style_fn=_get_style_for_value)

@joce joce merged commit d48bc23 into master Jan 11, 2026
15 checks passed
@joce joce deleted the feature/all-columns branch January 15, 2026 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants