fix: return fractional prices from commerce.price()

[ejcheng]

closes #350

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.56%. Comparing base (8e880c1) to head (ae6343d).

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #2458   +/-   ##
=======================================
  Coverage   99.56%   99.56%           
=======================================
  Files        2846     2846           
  Lines      248656   248715   +59     
  Branches      642      654   +12     
=======================================
+ Hits       247581   247643   +62     
+ Misses       1075     1072    -3     

Files	Coverage Δ
src/modules/commerce/index.ts	98.98% <100.00%> (+0.13%)	:arrow_up:

... and 2 files with indirect coverage changes

[ST-DDT]

There are some prices that are like 1.20€ like a serving of ice.

[ST-DDT]

Amazon Prime Video today:

But I must admit a price like 10.09€ is rare.

[matthewmayer]

I think a more realistic algorithm for prices might be:

• Generate the raw price
• Round to 2 or 3 significant digits (e.g. 1.23 or 12.30 or 123.00 or 1230.00)
• Optionally deduct 0.01 or 0.05

[ejcheng]

There are some prices that are like 1.20€ like a serving of ice.

Also, when you convert a price from one currency to another, you often get something like "$120.14".

[ST-DDT]

Team Decision

We will alter the distribution to favor the special numbers by using faker.helpers.weightedArrayElement for the last decimal digit.

• 9: weight: 5
• 5: weight: 3
• 0: weight: 1
• random(0-9): weight: 1

If the dec is zero we will skip this behavior.

[ejcheng]

While this is an improvement, these prices still don't seem to be that realistic for a typical e-commerce site.

faker.helpers.multiple(x=>faker.commerce.price(), {count:100})
[
  '820.09', '579.51', '33.55',  '852.35', '895.00', '177.55',
  '268.10', '506.25', '808.19', '473.33', '46.55',  '76.95',
  '767.89', '268.39', '575.25', '826.20', '315.69', '696.99',
  '41.40',  '969.69', '213.70', '518.15', '797.59', '786.95',
  '65.69',  '931.92', '553.30', '493.65', '174.99', '187.10',
  '778.49', '950.00', '633.40', '504.19', '221.79', '34.69',
  '717.75', '980.06', '207.55', '988.15', '498.39', '299.95',
  '267.72', '955.59', '276.47', '513.59', '16.99',  '300.19',
  '670.53', '9.39',   '908.79', '802.59', '397.59', '546.09',
  '25.69',  '475.09', '215.29', '240.89', '477.10', '919.55',
  '446.58', '239.45', '845.55', '826.70', '674.79', '22.69',
  '674.19', '508.95', '956.69', '733.32', '475.19', '21.49',
  '619.79', '84.69',  '619.89', '791.50', '880.19', '620.69',
  '93.90',  '443.49', '288.69', '813.81', '662.89', '232.95',
  '890.45', '251.89', '841.99', '795.40', '541.08', '715.89',
  '792.19', '646.99', '176.49', '782.49', '659.79', '482.65',
  '154.40', '455.39', '188.59', '894.79'
]

I think part of the issue is that the default max is 1000, and you're unlikely to find prices above 100 with 5 significant digits like 820.09.

Could consider

• Changing the default max to 100 or
• Overriding the last two digits using weightedArrayElements, favoring .00, .95, .99 and 10% random.

I'm in favor of overriding the last two digits.

[ST-DDT]

The last two decimal digits or the last two significant digits?

[ejcheng]

The last two decimal digits or the last two significant digits?

I would think decimal digits, but I'm open to significant digits as well.

[matthewmayer]

We might need more tests for the new behavior:

faker.seed(4);
faker.commerce.price({ dec: 2, min: 1, max: 1.1 }); // 1.15 ⚠️ 

To avoid this I'd do a final sanity check that the value with the replaced cents is still within min...max, and if it isn't use the original value instead.

[ST-DDT]

How about changing how the method is implemented.

const suffix: number = switch ...;
const random = faker.number.float(..., 1/10**(dec-1)) + suffix / 10 ** dec);
const clamped = Math.min....

return `${symbol}${clamped}`;

[ejcheng]

How about changing how the method is implemented.

const suffix: number = switch ...;
const random = faker.number.float(..., 1/10**(dec-1)) + suffix / 10 ** dec);
const clamped = Math....

return `${symbol}${clamped}`;

Sorry, what is suffix?

[ST-DDT]

Sorry, what is suffix?

The lastDigit. Instead of generating it, we omit it in the number generation and mathematically add it.

dec = 2 lastDigit = switch ...; // => 9 ( * 10^-2) generated = faker.number.float(precision: 10^-1); // => 12.1 combined => "$12.19"

[matthewmayer]

I'm still in favour of overriding the last two digits per my earlier review for more realistic prices.

[ST-DDT]

Team Decision

• For now we stay with a single altered digit.
• Before we touch it again we should do some research and maybe refactor the entire method to produce plausible prices without the need for the dec parameter.
• @ST-DDT will create an issue to rethink the price method
• "ChatGpt also doesn't think the 9s are important to include in the algorithm"

[ST-DDT]

Please add a test for:

faker.commerce.price({ min: 0.001, max: 0.009, dec: 3 })

Maybe fix it by wrapping the

faker.number.float({
    min,
    max,
    precision: (1 / 10) ** (dec - 1),
  })

with a try catch

try {
  generated = ...
} catch {
  return faker.number.float({
    min,
    max,
    precision: (1 / 10) ** dec,
  });
}

[ST-DDT]

@import-brain Could you please update this PR or should we take it over?

[ejcheng]

@import-brain Could you please update this PR or should we take it over?

@ST-DDT Could you please take this over?

[ST-DDT]

Sure no problem. This might take a bit though.

[ST-DDT]

Here a few prices that are returned by the price method:

Summary

Last Digit	Occurrences
0	13
1	0
2	1
3	1
4	2
5	30
6	1
7	1
8	2
9	49

Values

Price	Last Digit
278.39	9
224.59	9
171.99	9
968.26	6
832.10	0
911.79	9
345.35	5
842.69	9
636.79	9
325.29	9
666.80	0
891.55	5
897.49	9
377.09	9
731.65	5
821.55	5
650.40	0
595.09	9
268.18	8
461.00	0
188.25	5
915.30	0
483.49	9
1.19	9
203.95	5
611.29	9
546.49	9
297.95	5
759.49	9
344.69	9
388.89	9
898.15	5
319.95	5
99.25	5
399.69	9
269.45	5
562.29	9
336.45	5
628.39	9
24.99	9
89.65	5
380.05	5
183.99	9
647.03	3
896.95	5
37.45	5
581.39	9
70.79	9
281.10	0
878.29	9
838.05	5
940.95	5
692.09	9
938.20	0
936.89	9
339.45	5
663.55	5
664.19	9
663.59	9
234.55	5
609.69	9
817.09	9
448.55	5
755.69	9
514.15	5
406.72	2
368.75	5
375.50	0
377.60	0
633.69	9
813.09	9
281.39	9
834.69	9
639.95	5
729.39	9
91.50	0
497.29	9
281.49	9
771.09	9
803.69	9
159.85	5
386.79	9
738.77	7
442.85	5
210.74	4
569.78	8
171.84	4
206.19	9
732.79	9
305.89	9
181.55	5
311.39	9
694.30	0
413.45	5
317.80	0
483.20	0
599.19	9
765.69	9
34.89	9
507.65	5

[ST-DDT]

Ready for review

[matthewmayer]

While it's not strictly a breaking change I think it's worth a mention in the migration guide given that it changes the behavior of the method.

[ST-DDT]

While it's not strictly a breaking change I think it's worth a mention in the migration guide given that it changes the behavior of the method.

Added