Skip to content

feat: add unit formatter system with enums and conversion logic#679

Open
Axlrbs wants to merge 10 commits intomainfrom
features/add-unit-formatter-system
Open

feat: add unit formatter system with enums and conversion logic#679
Axlrbs wants to merge 10 commits intomainfrom
features/add-unit-formatter-system

Conversation

@Axlrbs
Copy link
Contributor

@Axlrbs Axlrbs commented Jan 27, 2026

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a comprehensive unit formatting system that enables automatic conversion and display of physical quantities across multiple unit families including energy, power, mass, distance, pressure, and more.

Changes:

  • Added unit enums (EnergyUnit, PowerUnit, MassUnit, etc.) with comprehensive coverage of physical quantities
  • Implemented unit registry with conversion factors, precision settings, and family-based relationships
  • Created useUnitFormatter composable with formatQuantity, formatQuantityParts, and convert functions
  • Added extensive test coverage (305 lines) covering family-based scaling, alternative conversions, edge cases, and locale formatting

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/shared/foundation-shared-domain/enums/units.ts Defines unit enums (EnergyUnit, PowerUnit, etc.), UnitFamily enum, and UnitPrefix enum for SI prefixes
src/shared/foundation-shared-domain/models/units/unitDetails.ts Defines TypeScript types for unit definitions including UnitDefinition, UnitConversion, and UnitScaleStep
src/shared/foundation-shared-domain/models/units/unitsRegistry.ts Implements the unit registry mapping unit strings to their definitions with conversion factors, precision, and families
src/shared/foundation-shared-domain/models/units/index.ts Exports unit-related models
src/shared/foundation-shared-domain/models/index.ts Adds units export to main models index
src/shared/foundation-shared-domain/enums/index.ts Adds units export to main enums index
src/shared/foundation-shared-services/config/units/unitPrefixes.ts Defines SI prefix configuration for handling unknown units
src/shared/foundation-shared-services/config/units/index.ts Exports unit configuration
src/shared/foundation-shared-services/config/index.ts Adds units export to main config index
src/shared/foundation-shared-services/composables/useUnitFormatter.ts Implements the core formatting logic with family-based conversions, alternative conversions, and SI prefix fallback
src/shared/foundation-shared-services/composables/index.ts Exports useUnitFormatter composable
tests/shared/units.test.ts Provides comprehensive test coverage for all formatting scenarios including edge cases and locale-specific formatting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@gballigand gballigand left a comment

Choose a reason for hiding this comment

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

Cas d'usages à refléchir :

  • Passer de °C à F
  • Passer de la famille Wh à Joules (mais il faut tout de même que ça fasse la mise à l'echelle sur la nouvelle famille)
  • On veut pouvoir fixer par exemple de m3 et que ça ne descendre jamais en litre mais que ça donne par exemple des milli m2

import { EnergyUnit, PowerUnit, GasFlowUnit, PressureUnit, TemperatureUnit, SpeedUnit, DistanceUnit, MassUnit, MassFlowUnit, FrequencyUnit, VoltageUnit, CurrentUnit, ResistanceUnit, PercentageUnit, CapacityUnit, SnowProductionUnit, EfficiencyUnit, VolumeUnit, GasVolumeUnit, WaterFlowUnit, UnitFamily } from "@dative-gpi/foundation-shared-domain/enums"
import type { UnitDefinition } from "./unitDetails"

export const unitRegistry: Record<string, UnitDefinition> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

si c'est automatique supprimé les cas qui sont générique (Wh par exemple)

family: UnitFamily.Volume,
factor: 1, // Pivot
},
[VolumeUnit.m3]: {
Copy link
Contributor

Choose a reason for hiding this comment

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

le M3 est l'unité SI du litre

- Introduced new functions for finding SI prefixes and parsing units with prefixes.
- Enhanced conversion logic to handle conversions within the same family and apply special conversions.
- Updated formatQuantity to support target units and unit precision, improving flexibility in formatting.
- Added comprehensive tests for various unit conversions, including energy, power, volume, and temperature.
- Ensured proper handling of edge cases, including zero, negative values, and unknown units.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 94 to 101
if (!unitSourceDefinition) {
const prefix = findBestSIPrefix(value);
const scaledValue = value / prefix.factor;
return {
value: scaledValue,
unit: `${prefix.prefix}${unit}`,
symbol: `${prefix.prefix}${unit}`
};
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The selectBestUnit function ignores the unitPrecision parameter when handling unknown units (lines 94-101). If a source unit is not in the registry, the function applies automatic SI prefix scaling without considering the unitPrecision option. This means the test on line 423-424 may not work as expected - the formatter might return a different prefix than "k" based on the value 123. Consider either documenting this limitation or extending the unknown unit handling to respect unitPrecision when provided.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A réfléchir...

// Apply special conversions
const afterSpecial = applySpecialConversions(value, unit, unitSourceDefinition);
if (afterSpecial.unit !== unit) {
return selectBestUnit(afterSpecial.value, afterSpecial.unit, unitPrecision);
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The recursive call to selectBestUnit on line 138 when a special conversion is applied could lead to infinite recursion if circular special conversions are defined in the unit registry (e.g., unit A converts to B, and B converts back to A under certain thresholds). Consider adding a safeguard such as a maximum recursion depth counter or a visited units set to prevent stack overflow errors.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A réfléchir...

- Introduced Area unit in UnitFamily and AreaUnit enums.
- Updated UnitDefinition to support optional exponent handling.
- Enhanced unitFamilies and unitRegistry to include Area unit.
- Implemented unit conversion functions to handle exponent-based conversions.
- Refactored useUnitFormatter to integrate new conversion logic and thresholds.
- Added comprehensive tests for unit conversions, including edge cases for thresholds and exponents.
@SchroterQuentin
Copy link
Member

J'ai pas eu le temps de te faire un retour mais j'ai regardé le code et c'est trop compliqué. On regarde ça ensemble quand on a un peu de temps.

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.

3 participants