feat: support custom field ordering and subsetting in Metric.write() #269

nh13 · 2025-12-16T06:14:17Z

Supersedes #178.

Adds two ways to customize which fields are written and in what order:

Class-level: Override _fields_to_write() for consistent ordering across all writes
Call-level: Pass include_fields or exclude_fields to write()

The call-level approach takes precedence, following the pattern of csv.DictWriter.

# Class-level: child fields before parent fields
@dataclass
class ChildMetric(ParentMetric):
    child_field: str

    @classmethod
    def _fields_to_write(cls, field_types: List[FieldType]) -> List[str]:
        return ["child_field", "parent_field"]

# Call-level: ad-hoc field selection
MyMetric.write(path, metric, include_fields=["field_a", "field_b"])
MyMetric.write(path, metric, exclude_fields=["internal_field"])

Fixes #177

coderabbitai · 2025-12-16T06:17:50Z

📝 Walkthrough

Walkthrough

Added class-level write ordering via new classmethod _fields_to_write(field_types) and made Metric.write(...) accept include_fields and exclude_fields (and threads). header() now derives its output from _fields_to_write and validates fields for existence and uniqueness. Write path propagates include/exclude to the MetricWriter and respects class-defined ordering when excluding or defaulting fields. Tests added to exercise reordering, subsetting, call-time overrides, roundtrip read/write, and header validation/duplicate error cases.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly summarizes the main change: custom field ordering and subsetting support added to Metric.write().
Description check	✅ Passed	Description explains both class-level and call-level mechanisms with concrete examples and references linked issue #177.
Linked Issues check	✅ Passed	PR implements the objective from #177 by adding _fields_to_write() for class-level field ordering and include_fields/exclude_fields parameters for call-level customization.
Out of Scope Changes check	✅ Passed	All changes are directly related to adding field ordering/subsetting: metric.py adds the core mechanism, test_metric.py adds comprehensive tests for the new functionality.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch nh13/feat-metric-field-ordering

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/fgpyo/util/test_metric.py (1)
997-997: Optional: Prefix unused parameter to silence linter.

The field_types parameter is required by the method signature but unused in these simple test implementations. Consider prefixing with _ to indicate intentional:
-def _fields_to_write(cls, field_types: List[FieldType]) -> List[str]:
+def _fields_to_write(cls, _field_types: List[FieldType]) -> List[str]:
Not a functional issue—just silences static analysis warnings.

Also applies to: 1026-1026, 1104-1104, 1135-1135, 1160-1160, 1177-1177, 1195-1195

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a1ffed0 and 9be384a.

📒 Files selected for processing (2)

fgpyo/util/metric.py (4 hunks)
tests/fgpyo/util/test_metric.py (2 hunks)

🧰 Additional context used

🪛 Ruff (0.14.8)

fgpyo/util/metric.py

412-414: Avoid specifying long messages outside the exception class

(TRY003)

419-421: Avoid specifying long messages outside the exception class

(TRY003)

tests/fgpyo/util/test_metric.py

997-997: Unused class method argument: field_types

(ARG003)

1026-1026: Unused class method argument: field_types

(ARG003)

1104-1104: Unused class method argument: field_types

(ARG003)

1135-1135: Unused class method argument: field_types

(ARG003)

1160-1160: Unused class method argument: field_types

(ARG003)

1177-1177: Unused class method argument: field_types

(ARG003)

1195-1195: Unused class method argument: field_types

(ARG003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Tests (3.9)

🔇 Additional comments (8)

tests/fgpyo/util/test_metric.py (2)

32-32: LGTM - required import for new feature.

The FieldType import is needed for the _fields_to_write() method signature in test fixtures.

987-1210: Excellent test coverage for the new field ordering features.

The nine new test functions comprehensively cover:

Field reordering and subsetting via _fields_to_write()

Call-level include_fields and exclude_fields

Precedence (include_fields overrides class-level ordering)

Roundtrip behavior with optional fields

Error cases (non-existent fields, duplicates)

Order preservation when using exclude_fields

All tests verify both header output and actual file contents, and are parametrized for both attr and dataclasses implementations.

fgpyo/util/metric.py (6)

114-149: Clear documentation with helpful examples.

The added documentation effectively explains both class-level (_fields_to_write()) and call-level (include_fields/exclude_fields) customization options, with practical guidance on when to use each approach.

370-401: Well-designed extension point.

The _fields_to_write() hook provides clean class-level customization. Default implementation preserves original behavior, and the docstring clearly distinguishes between class-level overrides and call-level parameters.

402-423: Robust validation logic.

The updated header() method correctly validates that _fields_to_write() returns only valid, non-duplicate field names. Catches configuration errors early.

339-368: Clean API extension.

Adding include_fields and exclude_fields parameters maintains backward compatibility while providing call-level control. The delegation to MetricWriter is straightforward.

717-726: Correct ordering implementation.

Using metric_class.header() instead of list(metric_class.keys()) ensures that exclude_fields respects the class-level field ordering from _fields_to_write(). This maintains the correct precedence hierarchy.

182-182: Required import for type hints.

The FieldType import is necessary for the _fields_to_write() method signature.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

fgpyo/util/metric.py (1)

416-421: Minor: O(n²) duplicate detection.

Using header.count(h) > 1 is O(n²). For small headers this is fine, but a Counter or seen-set approach would be O(n).

+        from collections import Counter
         # Validate no duplicates
-        if len(header) != len(set(header)):
-            duplicates = [h for h in header if header.count(h) > 1]
+        counts = Counter(header)
+        if len(header) != len(counts):
+            duplicates = [h for h, c in counts.items() if c > 1]
             raise ValueError(
                 f"_fields_to_write() returned duplicate fields: {', '.join(set(duplicates))}"
             )

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9be384a and 6dd86b7.

📒 Files selected for processing (2)

fgpyo/util/metric.py (4 hunks)
tests/fgpyo/util/test_metric.py (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

fgpyo/util/metric.py (2)

tests/fgpyo/util/test_metric.py (6)

_fields_to_write (174-175)

_fields_to_write (183-184)

_fields_to_write (199-200)

_fields_to_write (208-209)

_fields_to_write (1163-1164)

_fields_to_write (1180-1181)

fgpyo/util/inspect.py (1)

get_fields (404-413)

tests/fgpyo/util/test_metric.py (1)

fgpyo/util/metric.py (6)

Metric (204-536)

_fields_to_write (371-400)

header (403-423)

write (339-368)

write (652-675)

read (249-325)

🪛 Ruff (0.14.8)

fgpyo/util/metric.py

412-414: Avoid specifying long messages outside the exception class

(TRY003)

419-421: Avoid specifying long messages outside the exception class

(TRY003)

tests/fgpyo/util/test_metric.py

174-174: Unused class method argument: field_types

(ARG003)

183-183: Unused class method argument: field_types

(ARG003)

199-199: Unused class method argument: field_types

(ARG003)

208-208: Unused class method argument: field_types

(ARG003)

1163-1163: Unused class method argument: field_types

(ARG003)

1180-1180: Unused class method argument: field_types

(ARG003)

🔇 Additional comments (6)

tests/fgpyo/util/test_metric.py (2)

168-209: LGTM on new test metric classes.

These test variants properly exercise the _fields_to_write() API for reordering, subsetting, and error scenarios. The unused field_types parameter flagged by Ruff is intentional—these test implementations return hardcoded lists to verify specific behaviors while conforming to the interface signature.

1035-1205: Comprehensive test coverage for the new feature.

Tests cover reordering, subsetting, include/exclude parameters, precedence behavior, roundtrip with optional fields, and error cases. Good parametrization for both attr and dataclasses.

fgpyo/util/metric.py (4)

370-400: Well-designed extension point.

The _fields_to_write() method provides a clean hook for subclass customization with a sensible default. Documentation is thorough.

719-726: Correctly integrates class-level ordering.

Using header() for exclude and default cases ensures _fields_to_write() ordering is respected. include_fields correctly bypasses this to allow call-time override.

338-368: Clean API extension.

Parameters delegate to MetricWriter where validation occurs. Docstring clearly explains precedence behavior.

114-149: Excellent documentation.

Clear examples for both class-level and call-level customization with practical guidance on when to use each.

Fixes #177

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/fgpyo/util/test_metric.py (1)
168-210: Consider prefixing unused field_types with underscore.

The field_types parameter is unused in these test implementations (static analysis ARG003). Prefix with _ to indicate intentional non-use:
-def _fields_to_write(cls, field_types: List[FieldType]) -> List[str]:
+def _fields_to_write(cls, _field_types: List[FieldType]) -> List[str]:
Apply this pattern to all test metric classes (lines 174, 183, 199, 208).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6dd86b7 and bae093f.

📒 Files selected for processing (2)

fgpyo/util/metric.py (4 hunks)
tests/fgpyo/util/test_metric.py (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

tests/fgpyo/util/test_metric.py (1)

fgpyo/util/metric.py (5)

Metric (205-538)

_fields_to_write (372-401)

header (404-425)

write (340-369)

write (654-677)

fgpyo/util/metric.py (3)

tests/fgpyo/util/test_metric.py (6)

_fields_to_write (174-175)

_fields_to_write (183-184)

_fields_to_write (199-200)

_fields_to_write (208-209)

_fields_to_write (1163-1164)

_fields_to_write (1180-1181)

fgpyo/sam/builder.py (1)

header (586-588)

fgpyo/util/inspect.py (1)

get_fields (404-413)

🪛 Ruff (0.14.8)

tests/fgpyo/util/test_metric.py

174-174: Unused class method argument: field_types

(ARG003)

183-183: Unused class method argument: field_types

(ARG003)

199-199: Unused class method argument: field_types

(ARG003)

208-208: Unused class method argument: field_types

(ARG003)

1163-1163: Unused class method argument: field_types

(ARG003)

1180-1180: Unused class method argument: field_types

(ARG003)

fgpyo/util/metric.py

413-415: Avoid specifying long messages outside the exception class

(TRY003)

421-423: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (8)

fgpyo/util/metric.py (6)

154-154: LGTM: Imports are appropriate.

Both imports are used correctly in the implementation.

Also applies to: 183-183

114-149: LGTM: Documentation is clear and comprehensive.

The examples effectively illustrate when to use each approach.

340-369: LGTM: Backward-compatible signature extension.

The new parameters are optional and well-documented.

371-401: LGTM: Well-designed extension point.

The default implementation preserves existing behavior, and the docstring provides clear guidance on when to override.

403-425: LGTM: Validation logic is sound.

The validation catches invalid field references and duplicates. The static analysis TRY003 hints are acceptable here since error messages include dynamic field names for clarity.

695-730: LGTM: Internal ordering respects class-defined field order.

The use of header() ensures that _fields_to_write() customizations are honored throughout the write path.

tests/fgpyo/util/test_metric.py (2)

219-223: LGTM: Test metrics properly integrated.

All new metrics are added to the DataBuilder consistently.

1030-1205: LGTM: Comprehensive test coverage.

Tests cover reordering, subsetting, parameter interactions, validation errors, and roundtrip behavior. The parameterization ensures coverage for both attr and dataclasses implementations.

nh13 requested review from clintval and tfenne as code owners December 16, 2025 06:14

nh13 mentioned this pull request Dec 16, 2025

feat: support metric custom column ordering and sub-setting #178

Closed

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

coderabbitai bot approved these changes Dec 16, 2025

View reviewed changes

nh13 force-pushed the nh13/feat-metric-field-ordering branch from 9be384a to 6dd86b7 Compare December 16, 2025 06:19

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

feat: support custom field ordering and subsetting in Metric.write()

bae093f

Fixes #177

nh13 force-pushed the nh13/feat-metric-field-ordering branch from 6dd86b7 to bae093f Compare December 16, 2025 06:29

nh13 assigned clintval Dec 16, 2025

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

clintval approved these changes Dec 17, 2025

View reviewed changes

clintval assigned nh13 and unassigned clintval Dec 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: support custom field ordering and subsetting in Metric.write() #269

feat: support custom field ordering and subsetting in Metric.write() #269

nh13 commented Dec 16, 2025

Uh oh!

coderabbitai bot commented Dec 16, 2025 •

edited

Loading

Walkthrough

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: support custom field ordering and subsetting in Metric.write() #269

Are you sure you want to change the base?

feat: support custom field ordering and subsetting in Metric.write() #269

Conversation

nh13 commented Dec 16, 2025

Uh oh!

coderabbitai bot commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 16, 2025 •

edited

Loading