feat: convert nested obj with value and unit to number#2289
Open
feat: convert nested obj with value and unit to number#2289
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the array branch, the filter predicate excludes objects that have a
unit(i.e.!v.unit), which seems to contradict the description of handling{ value, unit }objects; consider always accepting objects with avalueproperty and ignoringunitrather than dropping those entries entirely. - You now duplicate the
typeof v === 'object' && v !== null && 'value' in vlogic in both the array handling and the scalarinnerValueextraction; consider extracting a small helper (e.g.unwrapValue(valueOrObject)), and reuse it in both branches for consistency and easier future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the array branch, the filter predicate excludes objects that have a `unit` (i.e. `!v.unit`), which seems to contradict the description of handling `{ value, unit }` objects; consider always accepting objects with a `value` property and ignoring `unit` rather than dropping those entries entirely.
- You now duplicate the `typeof v === 'object' && v !== null && 'value' in v` logic in both the array handling and the scalar `innerValue` extraction; consider extracting a small helper (e.g. `unwrapValue(valueOrObject)`), and reuse it in both branches for consistency and easier future changes.
## Individual Comments
### Comment 1
<location path="src/app/shared/pipes/format-number.pipe.ts" line_range="53-55" />
<code_context>
+ )
+ .map((v) => (typeof v === "object" ? v.value : v)),
);
+ const innerValue =
+ typeof value === "object" && value !== null && "value" in value
+ ? value.value
+ : value;
if (
</code_context>
<issue_to_address>
**question (bug_risk):** Behavior for objects with a `unit` property in arrays vs single values may be inconsistent.
In arrays, entries with a `unit` are excluded via `!v.unit`, while in the single-value path `{ value, unit? }` is accepted and `unit` is ignored. This asymmetry can lead to silently dropped values only in the array case. Consider making `unit` handling consistent across both branches (either always ignore it but keep the value, or consistently include/format it), so arrays and single values follow the same inclusion rules.
</issue_to_address>
### Comment 2
<location path="src/app/shared/pipes/format-number.pipe.ts" line_range="43" />
<code_context>
- typeof v === "bigint" ||
- typeof v === "string",
- ),
+ value
+ .filter(
+ (v) =>
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting helper functions to centralize unwrapping and filtering of `{ value; unit? }` and primitive inputs for both array and scalar cases.
You can keep the new `{ value; unit? }` support but reduce complexity by extracting small helpers and reusing them for both scalar and array paths.
### 1. Centralise unwrapping / filtering logic
Instead of inlining the complex `filter` and `innerValue` checks, pull them into helpers:
```ts
type ValueWithUnit = { value: number | string | bigint; unit?: string };
function isValueWithUnit(input: unknown): input is ValueWithUnit {
return (
typeof input === "object" &&
input !== null &&
"value" in input
);
}
function shouldIncludeInArray(input: unknown): input is string | number | bigint | ValueWithUnit {
if (typeof input === "number" || typeof input === "bigint" || typeof input === "string") {
return true;
}
// keep the existing behavior of excluding values that have a unit
return isValueWithUnit(input) && !input.unit;
}
function unwrapValue(input: string | number | bigint | ValueWithUnit): string | number | bigint {
return isValueWithUnit(input) ? input.value : input;
}
```
Then the array handling becomes simpler and the domain rule (`!unit`) is clearly named and isolated:
```ts
if (Array.isArray(value)) {
const normalized = value
.filter(shouldIncludeInArray)
.map(unwrapValue);
return String(normalized);
}
```
### 2. Reuse the same unwrapping for the scalar path
You can avoid the duplicated `innerValue` logic by reusing `unwrapValue`:
```ts
const innerValue = isValueWithUnit(value) ? value.value : value;
if (
typeof innerValue !== "string" &&
typeof innerValue !== "number" &&
typeof innerValue !== "bigint"
) {
return "";
}
```
This keeps functionality unchanged but:
- removes repeated `typeof === 'object' && value !== null && 'value' in value` checks,
- clearly isolates the “ignore items with unit” rule,
- makes it obvious what shapes are supported and how they’re normalized.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
d91aa24 to
dab48bc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Converts objs with "value", "unit" to number by concatenating the two. This is especially used when fetching scientificMetadata with mixed types
Summary by Sourcery
Support formatting of metadata values provided as objects with numeric/string values and optional units, including within arrays, while preserving existing scientific notation behavior.
New Features:
Tests: