- 
                Notifications
    You must be signed in to change notification settings 
- Fork 33
feat: format metadata floats with significant digits #2067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider parsing numeric strings into numbers in MetadataValueService so that string inputs representing numbers (e.g., from JSON) are also formatted according to the significant‐digits settings instead of returned verbatim.
- MetadataValueService currently reads the config only once in its constructor—if the float‐format toggle or values can change during runtime, you may need to re-initialize or subscribe to config updates so formatting stays in sync.
- Add unit tests for edge cases at the exact minCutoff and maxCutoff boundaries to confirm whether those values use decimal or scientific notation as intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider parsing numeric strings into numbers in MetadataValueService so that string inputs representing numbers (e.g., from JSON) are also formatted according to the significant‐digits settings instead of returned verbatim.
- MetadataValueService currently reads the config only once in its constructor—if the float‐format toggle or values can change during runtime, you may need to re-initialize or subscribe to config updates so formatting stays in sync.
- Add unit tests for edge cases at the exact minCutoff and maxCutoff boundaries to confirm whether those values use decimal or scientific notation as intended.
## Individual Comments
### Comment 1
<location> `src/app/shared/services/metadata-value.service.spec.ts:20-40` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 2
<location> `src/app/shared/services/metadata-value.service.ts:17` </location>
<code_context>
      const metadataFloatFormat = config.metadataFloatFormat;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
      const {metadataFloatFormat} = config;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Hi, first of all, the result looks a lot better than without this change! 
 I sadly missed the discussion in the last zoom meeting, but would it make sense to allow to track this on quantities as a next step and fall back to this general behaviour when the quantity lacks the accuracy? Best | 
| 
 Hi, Thanks for taking a look! 
 How did you configure it? If you only set metadataFloatFormatEnabled to true it will use default values for significant digits, minCutoff and maxCutoff. I agree with your proposal on a "precision" field next to value and unit. | 
| 
 Yeah sorry. When I wanted to try it out with a backend that was not local I ran into the thing again where the backend overrides the frontend config. Hasn't bitten me in a while and I forgot. So I thought there was a problem and I wanted to check with you. | 
Description
Add possibility to round floats with significant digits
If enabled, values in both the metadata-view and tree-view are rounded.
Motivation
Currently, all decimals for a float is shown which doesn't look good and is likely not helpful to anybody.
Changes:
If enabled, it is possibly to configure number of significant digits and two cutoff values for rounding float values.
If minCutoff < floatValue < maxCutoff, decimal format (i.e 0.123) would be used.
If the floatValue is outside of this range, scientific notation is used (i.e. 1.23e+6)
Default values if not provided is:
significantDigits: 3
minCutoff: 0.001
maxCutoff: 10000
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Enable configurable significant-digit formatting for float metadata values in both metadata and tree views, switching between decimal and scientific notation based on cutoff thresholds.
New Features:
Enhancements:
Build:
Tests: