Various Decimal usability tweaks #10517

GregoryTravis · 2024-07-10T20:10:31Z

Rename Decimal.from_string to Decimal.from_text.
Add a dec function to top level namespace
Integers are automatically converted to decimals when used in decimal operations
Arithmetic Decimal operations between a decimal and a float should throw an error
Equality
Should promote the float to a decimal and attach a warning (saying best to use decimals
Comparison Perform the comparison between the decimal and the float; Attach same warning as equality.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

jdunkerley · 2024-07-10T20:11:15Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso

@@ -89,7 +89,7 @@ type Decimal
    Value (big_decimal : BigDecimal)

    ## ICON input_number
-       Construct a `Decimal` from a string or integer.
+       Construct a `Decimal` from a string, integer or float.


Suggested change

Construct a `Decimal` from a string, integer or float.

Construct a `Decimal` from a Text, Integer or Float.

jdunkerley · 2024-07-10T20:11:25Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso

@@ -114,11 +114,21 @@ type Decimal
          Create a `Decimal` from a string.


Suggested change

Create a `Decimal` from a string.

Create a `Decimal` from a Text.

jdunkerley · 2024-07-10T20:14:22Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso

+
+            c = dec 12.345
+dec : Text | Integer | Float -> Math_Context | Nothing -> Decimal ! Arithmetic_Error | Number_Parse_Error
+dec (x : Text | Integer | Float) (mc : Math_Context | Nothing = Nothing) -> Decimal ! Arithmetic_Error | Number_Parse_Error =


I wonder what we should do with Math_Context | Nothing - won't play nicely with the widgets.
One to think about in the decimal refinement.

Shall we just add 2 constructors to Math_Context? e.g. Custom and Default?

radeusgd · 2024-07-11T08:40:23Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso


    ## PRIVATE
    internal_representation : [Integer]
-    internal_representation self = [self.unscaled_value, self.precision, self.scale]
+    internal_representation self -> [Integer] = [self.unscaled_value, self.precision, self.scale]


I don't think this is still valid. Probably accepted by the compiler, but I have no idea what it will do.

I think we should use

Suggested change

internal_representation self -> [Integer] = [self.unscaled_value, self.precision, self.scale]

internal_representation self -> Vector Integer = [self.unscaled_value, self.precision, self.scale]

instead, as that's what is used most of the time.

This is what happens when you write in other languages on the weekend.

radeusgd · 2024-07-11T08:41:03Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso

+    to_text_without_scientific_notation self -> Text = self.big_decimal.toPlainString
+
+## ICON input_number
+    Construct a `Decimal` from a string, integer or float.


I assume as @jdunkerley suggested we probably also want to replace string with Text here.

radeusgd · 2024-07-11T08:56:14Z

test/Base_Tests/src/Data/Decimal_Spec.enso

@@ -97,15 +105,15 @@ add_specs suite_builder =
            mc4 = Math_Context.new 4
            mc5 = Math_Context.new 5

-            Problems.not_expect_warning (Decimal.new "123.25")
+            Problems.not_expect_warning Any (Decimal.new "123.25")


not_expect_warning is especially useful if we do not expect a warning of type X but we acknowledge there may be other kinds of warnings.

If you expect no warnings at all, I'd recommend Problems.assume_no_problems

radeusgd · 2024-07-11T08:57:12Z

test/Base_Tests/src/Data/Decimal_Spec.enso

+                    Problems.expect_only_warning Loss_Of_Numeric_Precision (d < d)
+                    Problems.expect_only_warning Loss_Of_Numeric_Precision (d > d)
+                    Problems.expect_only_warning Loss_Of_Numeric_Precision (d <= d)
+                    Problems.expect_only_warning Loss_Of_Numeric_Precision (d >= d)


Why do we have these warnings here? I don't think I understand. I'm comparing a Decimal with Decimal - why would that give me a warning???

Yes, this is quite wrong and I missed it because of other problems with the test and with comparison. I marked this pending and it will be completed in #10163.

GregoryTravis added 7 commits July 8, 2024 11:47

from_string to from_text

5d7d552

dec

3d7ad7e

int/dec tests

58913e7

tests for f/d conversion warnings

d2f4e7e

test warnings on mixed comparisons

f230c60

return inline types

4191563

Merge branch 'develop' into wip/gmt/10480-tweaks

7e7c44f

GregoryTravis requested review from jdunkerley, radeusgd, AdRiley and marthasharkey as code owners July 10, 2024 20:10

merge

72cc079

jdunkerley approved these changes Jul 10, 2024

View reviewed changes

GregoryTravis mentioned this pull request Jul 10, 2024

Decimal tweaks #10480

Closed

5 tasks

radeusgd reviewed Jul 11, 2024

View reviewed changes

GregoryTravis added 3 commits July 11, 2024 10:44

Merge branch 'develop' into wip/gmt/10480-tweaks

ac986e2

review

4ad7d9c

disable mixed comparison test

11e2bd3

GregoryTravis added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 11, 2024

chaqngelog

64be2e5

GregoryTravis requested a review from radeusgd July 15, 2024 18:19

GregoryTravis added 6 commits July 15, 2024 16:30

wip

aaa9c67

wip

808c8ea

Merge branch 'wip/gmt/10480-tweaks' into wip/gmt/10163-d-comp

bde8831

merge

78304b9

wip

9cddd97

Merge branch 'wip/gmt/10480-tweaks' into wip/gmt/10163-d-comp

3fa7cc4

radeusgd approved these changes Jul 16, 2024

View reviewed changes

GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Jul 16, 2024

Merge branch 'develop' into wip/gmt/10480-tweaks

a894da7

mergify bot merged commit 0268cbb into develop Jul 16, 2024
36 checks passed

mergify bot deleted the wip/gmt/10480-tweaks branch July 16, 2024 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various Decimal usability tweaks #10517

Various Decimal usability tweaks #10517

GregoryTravis commented Jul 10, 2024 •

edited

Loading

jdunkerley Jul 10, 2024

GregoryTravis Jul 11, 2024

jdunkerley Jul 10, 2024

GregoryTravis Jul 11, 2024

jdunkerley Jul 10, 2024

radeusgd Jul 11, 2024

GregoryTravis Jul 11, 2024

radeusgd Jul 11, 2024

GregoryTravis Jul 11, 2024

radeusgd Jul 11, 2024

GregoryTravis Jul 11, 2024

radeusgd Jul 11, 2024

GregoryTravis Jul 11, 2024

radeusgd Jul 11, 2024

GregoryTravis Jul 11, 2024

	Construct a `Decimal` from a string, integer or float.
	Construct a `Decimal` from a Text, Integer or Float.

		@@ -114,11 +114,21 @@ type Decimal
		Create a `Decimal` from a string.

	Create a `Decimal` from a string.
	Create a `Decimal` from a Text.

	internal_representation self -> [Integer] = [self.unscaled_value, self.precision, self.scale]
	internal_representation self -> Vector Integer = [self.unscaled_value, self.precision, self.scale]

Various Decimal usability tweaks #10517

Various Decimal usability tweaks #10517

Conversation

GregoryTravis commented Jul 10, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregoryTravis commented Jul 10, 2024 •

edited

Loading