feat: trim flanking whitespace when reading a metric #217
Conversation
WalkthroughThe pull request modifies the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fgpyo/util/metric.py (1)
356-356: Add docstring to document whitespace stripping behavior.The new whitespace stripping behavior should be documented in the method's docstring.
Add this to the docstring:
Args: value: the value to format. + + Note: + Leading and trailing whitespace is stripped from all string representations.Also applies to: 360-360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/metric.py(1 hunks)tests/fgpyo/util/test_metric.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python package
tests/fgpyo/util/test_metric.py
[error] 408-408: Test failure in test_metric_whitespace_padded_strip: String comparison mismatch. Expected 'John Doe' but got ' John Doe \t ' with extra whitespace and tab characters.
tests/fgpyo/util/test_metric.py
Outdated
| @pytest.mark.parametrize("data_and_classes", (attr_data_and_classes, dataclasses_data_and_classes)) | ||
| def test_metric_whitespace_padded_strip(data_and_classes: DataBuilder) -> None: | ||
| assert list(data_and_classes.Person(name=" John Doe \t ", age=42).values()) == ["John Doe", 42] | ||
|
|
There was a problem hiding this comment.
Fix test failure: values not being stripped.
Test expects whitespace stripping but fails. The values() method returns raw values, while formatted_values() returns formatted (stripped) values.
Fix the test by using formatted_values():
- assert list(data_and_classes.Person(name=" John Doe \t ", age=42).values()) == ["John Doe", 42]
+ assert data_and_classes.Person(name=" John Doe \t ", age=42).formatted_values() == ["John Doe", "42"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize("data_and_classes", (attr_data_and_classes, dataclasses_data_and_classes)) | |
| def test_metric_whitespace_padded_strip(data_and_classes: DataBuilder) -> None: | |
| assert list(data_and_classes.Person(name=" John Doe \t ", age=42).values()) == ["John Doe", 42] | |
| @pytest.mark.parametrize("data_and_classes", (attr_data_and_classes, dataclasses_data_and_classes)) | |
| def test_metric_whitespace_padded_strip(data_and_classes: DataBuilder) -> None: | |
| assert data_and_classes.Person(name=" John Doe \t ", age=42).formatted_values() == ["John Doe", "42"] | |
🧰 Tools
🪛 GitHub Actions: Python package
[error] 408-408: Test failure in test_metric_whitespace_padded_strip: String comparison mismatch. Expected 'John Doe' but got ' John Doe \t ' with extra whitespace and tab characters.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 90.72% 90.81% +0.09%
==========================================
Files 18 18
Lines 2318 2320 +2
Branches 343 344 +1
==========================================
+ Hits 2103 2107 +4
+ Misses 145 144 -1
+ Partials 70 69 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fgpyo/util/metric.py (1)
213-213: Fix line length.Line exceeds maximum length of 100 characters.
- def read(cls, path: Path, ignore_extra_fields: bool = True, strip_whitespace: bool = True) -> Iterator[Any]: + def read( + cls, + path: Path, + ignore_extra_fields: bool = True, + strip_whitespace: bool = True, + ) -> Iterator[Any]:🧰 Tools
🪛 Ruff (0.8.2)
213-213: Line too long (112 > 100)
(E501)
🪛 GitHub Actions: Python package
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/metric.py(4 hunks)tests/fgpyo/util/test_metric.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python package
tests/fgpyo/util/test_metric.py
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
fgpyo/util/metric.py
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
🪛 Ruff (0.8.2)
fgpyo/util/metric.py
213-213: Line too long (112 > 100)
(E501)
🔇 Additional comments (5)
fgpyo/util/metric.py (2)
267-268: LGTM!Clean implementation of whitespace stripping with proper conditional check.
🧰 Tools
🪛 GitHub Actions: Python package
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
359-363: LGTM!Clean implementation of whitespace handling in value formatting.
🧰 Tools
🪛 GitHub Actions: Python package
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
tests/fgpyo/util/test_metric.py (3)
157-161: LGTM!Clean implementation of float age support.
🧰 Tools
🪛 GitHub Actions: Python package
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
411-419: LGTM!Test properly verifies float rounding behavior.
🧰 Tools
🪛 GitHub Actions: Python package
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
421-438: LGTM!Comprehensive test coverage for whitespace stripping functionality.
🧰 Tools
🪛 GitHub Actions: Python package
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
nh13
force-pushed
the
feat/strip-metric-values
branch
from
3d73db1 to
de3b0c7
Compare
nh13
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/fgpyo/util/test_metric.py (1)
411-417: Add edge cases to float rounding test.Consider adding test cases for:
- Very small numbers (e.g., 0.0000001)
- Very large numbers (e.g., 1e10)
- Negative numbers
@pytest.mark.parametrize("data_and_classes", (attr_data_and_classes, dataclasses_data_and_classes)) def test_metric_round_floats(data_and_classes: DataBuilder) -> None: - assert list(data_and_classes.Person(name="John Doe", age=42.123456).formatted_values()) == [ - "John Doe", - "42.12346", - ] + test_cases = [ + (42.123456, "42.12346"), + (0.0000001, "0.0"), + (1e10, "10000000000.0"), + (-42.123456, "-42.12346"), + ] + for value, expected in test_cases: + assert list(data_and_classes.Person(name="John Doe", age=value).formatted_values()) == [ + "John Doe", + expected, + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/metric.py(4 hunks)tests/fgpyo/util/test_metric.py(2 hunks)
🔇 Additional comments (5)
fgpyo/util/metric.py (3)
213-215: LGTM! Well-documented parameter addition.The new
strip_whitespaceparameter is properly documented and maintains backward compatibility.Also applies to: 226-226
269-270: LGTM! Clean implementation of whitespace stripping.The implementation correctly handles the
strip_whitespaceparameter.
361-361: LGTM! Consistent whitespace handling.The changes ensure uniform whitespace handling for both float and general values.
Also applies to: 365-365
tests/fgpyo/util/test_metric.py (2)
157-161: LGTM! Well-structured test class.The
PersonAgeFloatclass is properly implemented for testing float values.
419-436: LGTM! Comprehensive whitespace handling test.Test covers all important whitespace scenarios: leading, trailing, and both.
nh13
changed the title
tests/fgpyo/util/test_metric.py
Outdated
| assert persons[2].name == "Someone Else" | ||
| assert persons[2].age == 47 | ||
|
|
||
| persons = list(data_and_classes.Person.read(test_tsv, strip_whitespace=False)) |
There was a problem hiding this comment.
🙌 good catch to cover both cases!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fgpyo/util/metric.py (1)
226-226: Line exceeds maximum length of 100 characters.Current line is 113 characters long according to static analysis.
- strip_whitespace: True to strip leading and trailing whitespace from each field, False to keep as-is. + strip_whitespace: True to strip leading/trailing whitespace from each field, False to keep as-is.🧰 Tools
🪛 Ruff (0.8.2)
226-226: Line too long (113 > 100)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/metric.py(4 hunks)tests/fgpyo/util/test_metric.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fgpyo/util/test_metric.py
🧰 Additional context used
🪛 Ruff (0.8.2)
fgpyo/util/metric.py
226-226: Line too long (113 > 100)
(E501)
🔇 Additional comments (4)
fgpyo/util/metric.py (4)
214-215: Good choice usingFalseas default forstrip_whitespace.Setting the default to
Falsemaintains backward compatibility with existing code, preventing potential breaking changes.
269-270: Implementation correctly handles whitespace stripping.The code properly applies stripping only when requested and iterates through all values in the list.
361-361: The f-string formatting for float values looks good.Rounding to 5 decimal places is maintained while using modern f-string syntax.
365-365: Consistent use of f-strings for value formatting.Updated to use modern f-string notation for general values as well.
Fixes #216 Also, strips any flanking whitespace when formatting a float into a string.
nh13
force-pushed
the
feat/strip-metric-values
branch
from
20f469d to
faac2ac
Compare
Co-authored-by: Matt Stone <matt@fulcrumgenomics.com>
nh13
force-pushed
the
feat/strip-metric-values
branch
from
faac2ac to
a1de7d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fgpyo/util/metric.py (1)
226-226: Line exceeds length limit.Line is 113 characters, exceeding 100 character limit.
- strip_whitespace: True to strip leading and trailing whitespace from each field, False to keep as-is. + strip_whitespace: True to strip leading/trailing whitespace from each field, + False to keep as-is.🧰 Tools
🪛 Ruff (0.8.2)
226-226: Line too long (113 > 100)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/metric.py(4 hunks)tests/fgpyo/util/test_metric.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fgpyo/util/test_metric.py
🧰 Additional context used
🪛 Ruff (0.8.2)
fgpyo/util/metric.py
226-226: Line too long (113 > 100)
(E501)
🔇 Additional comments (4)
fgpyo/util/metric.py (4)
214-215: Appropriate default for compatibility.Good choice setting
strip_whitespaceto False by default. This maintains backward compatibility and avoids breaking existing code.
269-270: Clean implementation of whitespace stripping.The code correctly applies stripping only when requested, with a clear implementation.
361-361: Cleaner float formatting with f-string.The f-string approach is more readable than string concatenation.
365-365: Consistent string formatting.Good use of f-string for general values, matching the style used for float values.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fgpyo/util/metric.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fgpyo/util/metric.py
226-226: Trailing whitespace
Remove trailing whitespace
(W291)
🪛 GitHub Actions: test
fgpyo/util/metric.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Please run 'ruff format' to fix code style issues in this file.
🔇 Additional comments (4)
fgpyo/util/metric.py (4)
213-215: Parameter added correctly with safe default.The new
strip_whitespaceparameter with a default ofFalsemaintains backward compatibility, good approach.
270-271: Effective implementation of whitespace stripping.The conditional check and list comprehension is an efficient approach to handle the whitespace stripping.
362-362: Good use of f-string for float formatting.The f-string approach for formatting floats is clean and efficient.
366-366: Good use of f-string for general value formatting.Converting to f-string for general values is consistent with the float formatting approach.
Fixes #216
Also, strips any flanking whitespace when formatting a float into a string.