Skip to content
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

Conversions from Decimal to Integer and Float. #9462

Merged
merged 25 commits into from
Mar 30, 2024

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Mar 18, 2024

Pull Request Description

Adds .to_integer and .to_float, as well as from conversions.

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,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

huge_a_int = Integer.from huge_a

(huge_a_int == huge_a) . should_be_true # succeeds
(huge_a == huge_a_int) . should_be_true # fails
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JaroslavTulach I suspect that I am improperly returning a value from .to_integer here, resulting in this failure.

Copy link
Member

Choose a reason for hiding this comment

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

huge_a_int shall be converted to Decimal and the decimal comparator shall be invoked to compute hash and compare. If that's not happening, only debugger can tell us what's going on.

Copy link
Member

@JaroslavTulach JaroslavTulach Mar 21, 2024

Choose a reason for hiding this comment

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

The code to reproduce the problem is

from Standard.Base import all

main =
    huge_a = Decimal.new "3.4E320"
    huge_a_int = Integer.from huge_a

    (huge_a == huge_a_int)

The selected specialization is EqualsSimpleNode.equalsDifferent. It is true that the "primitiveness" of both values is different:

JavaObject[340000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 (java.math.BigInteger)]
isPrimitive(other, interop)
true
isPrimitive(self, interop)
false
self
Value JavaObject[3.4E+320 (java.math.BigDecimal)]

but there is a conversion between one and another - e.g. we should be continuing to EqualsNode.findConversions. Which we do, but then the conversion between the values fails.

I believe it is because InvokeConversionNode is using TypesLibrary, while we should TypeOfNode, I think (right now)!

Copy link
Member

Choose a reason for hiding this comment

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

This is a possible fix:

diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeConversionNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeConversionNode.java
index 6e03c9c10f..5afb6a0055 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeConversionNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeConversionNode.java
@@ -11,14 +11,15 @@ import org.enso.interpreter.node.BaseNode;
 import org.enso.interpreter.node.callable.dispatch.IndirectInvokeFunctionNode;
 import org.enso.interpreter.node.callable.resolver.ConversionResolverNode;
 import org.enso.interpreter.node.callable.resolver.HostMethodCallNode;
+import org.enso.interpreter.node.expression.builtin.meta.TypeOfNode;
 import org.enso.interpreter.runtime.EnsoContext;
 import org.enso.interpreter.runtime.callable.UnresolvedConversion;
 import org.enso.interpreter.runtime.callable.argument.CallArgumentInfo;
 import org.enso.interpreter.runtime.callable.function.Function;
 import org.enso.interpreter.runtime.data.ArrayRope;
+import org.enso.interpreter.runtime.data.Type;
 import org.enso.interpreter.runtime.data.text.Text;
 import org.enso.interpreter.runtime.error.*;
-import org.enso.interpreter.runtime.library.dispatch.TypesLibrary;
 
 @GenerateUncached
 @ReportPolymorphism
@@ -45,7 +46,11 @@ abstract class IndirectInvokeConversionNode extends Node {
       BaseNode.TailStatus isTail,
       int thatArgumentPosition);
 
-  @Specialization(guards = {"typesLib.hasType(that)", "!typesLib.hasSpecialDispatch(that)"})
+  static boolean hasType(TypeOfNode typesLib, Object value) {
+    return typesLib.execute(value) instanceof Type;
+  }
+
+  @Specialization(guards = {"hasType(typesLib, that)"})
   Object doConvertFrom(
       MaterializedFrame frame,
       Object state,
@@ -58,13 +63,16 @@ abstract class IndirectInvokeConversionNode extends Node {
       InvokeCallableNode.ArgumentsExecutionMode argumentsExecutionMode,
       BaseNode.TailStatus isTail,
       int thatArgumentPosition,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
+      @Shared("typesLib") @Cached TypeOfNode typesLib,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode,
       @Shared("indirectInvokeFunctionNode") @Cached
           IndirectInvokeFunctionNode indirectInvokeFunctionNode) {
     Function function =
         conversionResolverNode.expectNonNull(
-            that, InvokeConversionNode.extractType(this, self), typesLib.getType(that), conversion);
+            that,
+            InvokeConversionNode.extractType(this, self),
+            (Type) typesLib.execute(that),
+            conversion);
     return indirectInvokeFunctionNode.execute(
         function,
         frame,
@@ -201,12 +209,7 @@ abstract class IndirectInvokeConversionNode extends Node {
     }
   }
 
-  @Specialization(
-      guards = {
-        "!methods.hasType(that)",
-        "!interop.isString(that)",
-        "!methods.hasSpecialDispatch(that)"
-      })
+  @Specialization(guards = {"hasType(methods, that)", "!interop.isString(that)"})
   Object doFallback(
       MaterializedFrame frame,
       Object state,
@@ -219,7 +222,7 @@ abstract class IndirectInvokeConversionNode extends Node {
       InvokeCallableNode.ArgumentsExecutionMode argumentsExecutionMode,
       BaseNode.TailStatus isTail,
       int thatArgumentPosition,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary methods,
+      @Shared("typesLib") @Cached TypeOfNode methods,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop) {
     throw new PanicException(
         EnsoContext.get(this).getBuiltins().error().makeNoSuchConversion(self, that, conversion),
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java
index 6efd572037..16ca5a626c 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java
@@ -15,6 +15,7 @@ import java.util.concurrent.locks.Lock;
 import org.enso.interpreter.node.BaseNode;
 import org.enso.interpreter.node.callable.dispatch.InvokeFunctionNode;
 import org.enso.interpreter.node.callable.resolver.ConversionResolverNode;
+import org.enso.interpreter.node.expression.builtin.meta.TypeOfNode;
 import org.enso.interpreter.runtime.EnsoContext;
 import org.enso.interpreter.runtime.callable.UnresolvedConversion;
 import org.enso.interpreter.runtime.callable.argument.CallArgumentInfo;
@@ -29,7 +30,6 @@ import org.enso.interpreter.runtime.error.PanicException;
 import org.enso.interpreter.runtime.error.PanicSentinel;
 import org.enso.interpreter.runtime.error.Warning;
 import org.enso.interpreter.runtime.error.WithWarnings;
-import org.enso.interpreter.runtime.library.dispatch.TypesLibrary;
 import org.enso.interpreter.runtime.state.State;
 
 public abstract class InvokeConversionNode extends BaseNode {
@@ -99,7 +99,11 @@ public abstract class InvokeConversionNode extends BaseNode {
     return extractType(this, self);
   }
 
-  @Specialization(guards = {"dispatch.hasType(that)", "!dispatch.hasSpecialDispatch(that)"})
+  static boolean hasType(TypeOfNode typesLib, Object value) {
+    return typesLib.execute(value) instanceof Type;
+  }
+
+  @Specialization(guards = {"hasType(dispatch, that)"})
   Object doConvertFrom(
       VirtualFrame frame,
       State state,
@@ -107,9 +111,9 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object self,
       Object that,
       Object[] arguments,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary dispatch,
+      @Shared("typesLib") @Cached TypeOfNode dispatch,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode resolveNode) {
-    var thatType = dispatch.getType(that);
+    var thatType = (Type) dispatch.execute(that);
     if (thatType == self) {
       return that;
     } else {
@@ -127,7 +131,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object self,
       DataflowError that,
       Object[] arguments,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary dispatch,
+      @Shared("typesLib") @Cached TypeOfNode dispatch,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     Function function =
         conversionResolverNode.execute(
@@ -234,8 +238,7 @@ public abstract class InvokeConversionNode extends BaseNode {
 
   @Specialization(
       guards = {
-        "!typesLib.hasType(that)",
-        "!typesLib.hasSpecialDispatch(that)",
+        "!hasType(typesLib, that)",
         "!interop.isTime(that)",
         "interop.isDate(that)",
       })
@@ -247,7 +250,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object that,
       Object[] arguments,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
+      @Shared("typesLib") @Cached TypeOfNode typesLib,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     Function function =
         conversionResolverNode.expectNonNull(
@@ -257,8 +260,7 @@ public abstract class InvokeConversionNode extends BaseNode {
 
   @Specialization(
       guards = {
-        "!typesLib.hasType(that)",
-        "!typesLib.hasSpecialDispatch(that)",
+        "!hasType(typesLib, that)",
         "interop.isTime(that)",
         "!interop.isDate(that)",
       })
@@ -270,7 +272,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object that,
       Object[] arguments,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
+      @Shared("typesLib") @Cached TypeOfNode typesLib,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     Function function =
         conversionResolverNode.expectNonNull(
@@ -280,8 +282,7 @@ public abstract class InvokeConversionNode extends BaseNode {
 
   @Specialization(
       guards = {
-        "!typesLib.hasType(that)",
-        "!typesLib.hasSpecialDispatch(that)",
+        "!hasType(typesLib, that)",
         "interop.isTime(that)",
         "interop.isDate(that)",
       })
@@ -293,7 +294,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object that,
       Object[] arguments,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
+      @Shared("typesLib") @Cached TypeOfNode typesLib,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     Function function =
         conversionResolverNode.expectNonNull(
@@ -303,8 +304,7 @@ public abstract class InvokeConversionNode extends BaseNode {
 
   @Specialization(
       guards = {
-        "!typesLib.hasType(that)",
-        "!typesLib.hasSpecialDispatch(that)",
+        "!hasType(typesLib, that)",
         "interop.isDuration(that)",
       })
   Object doConvertDuration(
@@ -315,7 +315,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object that,
       Object[] arguments,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
+      @Shared("typesLib") @Cached TypeOfNode typesLib,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     Function function =
         conversionResolverNode.expectNonNull(
@@ -325,8 +325,7 @@ public abstract class InvokeConversionNode extends BaseNode {
 
   @Specialization(
       guards = {
-        "!typesLib.hasType(thatMap)",
-        "!typesLib.hasSpecialDispatch(thatMap)",
+        "!hasType(typesLib, thatMap)",
         "interop.hasHashEntries(thatMap)",
       })
   Object doConvertMap(
@@ -337,7 +336,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object thatMap,
       Object[] arguments,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
+      @Shared("typesLib") @Cached TypeOfNode typesLib,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     Function function =
         conversionResolverNode.expectNonNull(
@@ -345,12 +344,7 @@ public abstract class InvokeConversionNode extends BaseNode {
     return invokeFunctionNode.execute(function, frame, state, arguments);
   }
 
-  @Specialization(
-      guards = {
-        "!methods.hasType(that)",
-        "!interop.isString(that)",
-        "!methods.hasSpecialDispatch(that)"
-      })
+  @Specialization(guards = {"!hasType(methods, that)", "!interop.isString(that)"})
   Object doFallback(
       VirtualFrame frame,
       State state,
@@ -358,7 +352,7 @@ public abstract class InvokeConversionNode extends BaseNode {
       Object self,
       Object that,
       Object[] arguments,
-      @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary methods,
+      @Shared("typesLib") @Cached TypeOfNode methods,
       @Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
       @Shared("conversionResolverNode") @Cached ConversionResolverNode conversionResolverNode) {
     var ctx = EnsoContext.get(this);

e.g. the problem is truly in inconsistency of TypeOfNode and TypesLibrary - the differences may have wide implications. The real fix should:

  • either remove TypesLibrary public methods and use TypeOfNode everywhere
  • or move TypeOfNode behavior to default implementation of TypesLibrary and remove direct usages of TypeOfNode

Copy link
Member

Choose a reason for hiding this comment

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

Trying the alternative approach of moving TypeOfNode behavior to TypesLibrary here - not yet fully working, but also not fully broken.

Copy link
Member

Choose a reason for hiding this comment

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

Trying the alternative approach of moving TypeOfNode behavior to TypesLibrary here - not yet fully working, but also not fully broken.

@JaroslavTulach Why not the other way around? That is, make the TypeOfNode the default way to fetch the type. I thought that using nodes is more light-weight than using libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JaroslavTulach This patch does fix the failing test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug for this: #9547

@GregoryTravis GregoryTravis marked this pull request as ready for review March 18, 2024 19:14
Comment on lines 531 to 535
> Example
Convert `Decimal` 2345 to an `Integer`.
d = Decimal.new "2345"
d.to_integer
# => 2345
Copy link
Member

Choose a reason for hiding this comment

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

I'd like us to have an example that shows some more interesting case, e.g. 2345.6 to see what happens with the decimal part? Is it truncated, rounded? Is it only possible to convert if decimal part is .0 and it fails otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (It succeeds either way.)

Comment on lines 539 to 549
## GROUP CONVERSIONS
Convert this to a `Float`.

> Example
Convert `Decimal` 23.45 to a `Float`.

d = Decimal.new "23.45"
d.to_float
# => 23.45
to_float : Integer
to_float self = self.big_decimal.doubleValue
Copy link
Member

Choose a reason for hiding this comment

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

IMO it would be good to say what happens if the Decimal value is too large (IIRC it gets converted to Infinity, right?) - this is not clear and we should clearly state it. Also IMHO in such cases we should be attaching a warning (ideally I'd fail but I know that'd be too annyoing, IMO warning may be a decent compromise between usability and correctness).

Copy link
Member

Choose a reason for hiding this comment

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

I know it's rather obvious, but still I'd add a note that the conversion is not precise (because Float is not precise).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done -- attached a warning for both. If we are going to always attach a warning for from_float, it makes sense to do the same for to_float, since it's even more likely that there will be a loss of precision in the case of to_float.

Comment on lines 532 to 533
Convert `Decimal` 2345 to an `Integer`.
d = Decimal.new "2345"
Convert `Decimal` 2345.6 to an `Integer`.
d = Decimal.new "2345.6"
Copy link
Member

@radeusgd radeusgd Mar 19, 2024

Choose a reason for hiding this comment

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

So the behaviour is truncate? IMO it would be good to specify this clearly in the description. The example is good, but it may mean truncate as well as floor, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documented and added test.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Please make sure the tests are executed on CI.

@@ -582,3 +630,9 @@ Decimal.from (that : Text) = Decimal.parse that

## PRIVATE
Decimal.from (that : Number) = Decimal.new that

## PRIVATE
Copy link
Member

Choose a reason for hiding this comment

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

Why are these conversions marked as ## PRIVATE? I believe suggestion database doesn't contain from conversion methods, so the IDE shouldn't display these methods in the component browser anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

(huge_a_float == huge_a) . should_be_true
(huge_a == huge_a_float) . should_be_true

group_builder.specify "Decimal.to_integer should compare correctly with the original Decimal" pending="Decimal.to_integer comparison" <|
Copy link
Member

Choose a reason for hiding this comment

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

I cannot run this test as:

enso$ ./built-distribution/enso-engine-*/enso-*/bin/enso --run test/Base_Tests/ "Decimal.to_integer"

probably because Decimal_Spec isn't listed in Main.enso. Is it even executed on the CI? I don't see Decimal.to_integer anywhere in the log output!

Copy link
Member

Choose a reason for hiding this comment

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

It is listed in

Decimal_Spec.add_specs suite_builder
so it is included in all the tests. But it is still marked as pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@@ -176,14 +176,22 @@ private static boolean findConversionImpl(
UnresolvedConversion.build(selfScope).resolveFor(ctx, comparableType, thatType);
Copy link
Member

Choose a reason for hiding this comment

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

[Question not related to the current PR] Why is this conversion built with selfScope and not thatScope? We are looking for a conversion from thatType to Comparable type in selfScope, but shouldn't we be looking for this conversion in a different scope? Is is possible that this is some leftover? @JaroslavTulach

d.to_float
# => 23.45
to_float : Integer
to_float self =
Copy link
Member

Choose a reason for hiding this comment

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

Why there is dec.to_float when there also is a conversion and one can write dec.to Float?

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea was that currently there are no good completions for .to conversions - the IDE will not easily tell you which conversions are available and which ones are not. Having the explicit ones allows it to show up in the Component Browser.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Mar 29, 2024

@GregoryTravis I've just added my fix of #9547 inquiry here as de1228c. Should my fix cause any problems, feel free to remove the commit from this PR. I am sorry, if my fix causes issues, but this branch has the best reproducer and it is easier for me to fix the issue here than create my own reproducer elsewhere or wait while this PR gets merged.

@@ -247,7 +250,7 @@ Object doConvertDate(
Object that,
Object[] arguments,
@Shared("interop") @CachedLibrary(limit = "10") InteropLibrary interop,
@Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib,
@Shared("typesLib") @Cached TypeOfNode typesLib,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Shared("typesLib") @Cached TypeOfNode typesLib,
@Shared("typesLib") @Cached TypeOfNode typeOfNode,

@JaroslavTulach The type has changed but the name remains as before - shouldn't it be updated?

IMO having names that are inconsistent with what the value is, is likely to introduce confusion.

Copy link
Member

@JaroslavTulach JaroslavTulach Mar 29, 2024

Choose a reason for hiding this comment

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

Names of local variables don't have any effect on the execution. Your suggestion cannot be applied on its own - it would make the code uncompilable.

Copy link
Member

Choose a reason for hiding this comment

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

Names of local variables don't have any effect on the execution.

I know! I'm only complaining about code style. I find such small practices really greatly influencing how easy it is for me to understand the code. The variable name tells something. Sometimes it doesn't say much useful information than the type. But in cases like here, when there's inconsistency - I instantly start to think why the name is the way it is. In the diff I know - for historical reasons, because during refactor the variable names were not updated. But if I will come looking at this code later, without all the historical context - I will get confused and will be wondering what does this unusual name mean - why this instance is a Lib if its type suggests it's a Node - what is happening here? And I will lose time unproductively wondering what has happened here.

I know I'm saying as if this is a big thing. But for me it is. Of course a single variable name is not going to cause harm, but if there's a lot of such situations throughout the codebase, I start wasting more and more time trying to understand what the author meant, and processing the codebase when trying to extend it or fix something takes significantly longer when such things add up.

I guess you are familiar with this part of the codebase, so these things are obvious to you. But if I'm less familiar with it, even such minor details do affect how hard it is to understand pieces of the code. Do you really never experience e.g. bad naming causing confusion and causing you to take more time to understand the codebase?

I once heard a wise saying that code is read more times than it is written - so IMO taking a little bit of extra effort in making it easy to understand greatly pays off in the long term.

I really don't want to be complaining all the time about this, I hope the tone of my complaint above is not too offensive. I just really wanted to share how very frustrated I get when I have a hard time understanding parts of the codebase littered with cryptic variable names or little documentation.

Your suggestion cannot be applied on its own - it would make the code uncompilable.

Sorry, I used suggestion to show how the rename can be applied, but I know we need to update other occurrences. I felt it's enough to suggest what rename is needed, not the whole patch. I may try adding better suggestions next time, but it's not trivial to find all usages in the GitHub review, whereas with any IDE such rename is usually much easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JaroslavTulach this fix worked, thank you very much.
@radeusgd I'll do the renaming.

@JaroslavTulach JaroslavTulach force-pushed the wip/gmt/8982-conversion-int-float branch from 8e4a259 to de1228c Compare March 29, 2024 13:19
Comment on lines +505 to +507
to_display_text : Text
to_display_text self =
"Value out of range: "+self.value.to_text
Copy link
Member

Choose a reason for hiding this comment

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

IMHO it would be good to indicate what range we exceed, e.g.
Value out of representable Float range: ... in this case.
Do you think we can add this to the error info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #9586.

Problems.expect_only_warning Out_Of_Range f
f2 = (-huge_a).to_float
f2.should_equal Number.negative_infinity
Problems.expect_only_warning Out_Of_Range f2
Copy link
Member

Choose a reason for hiding this comment

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

please test the to_display_text of the warning.

We often used to have typos inside of to_display_text, especially after refacors. If it's not unit tested, we only learn about it when the warning/error comes up in the GUI and is scrambled. Better to have tests for this since we don't have much static analysis capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #9586.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good, just 2 small comments

@enso-bot enso-bot bot mentioned this pull request Mar 29, 2024
4 tasks
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Mar 30, 2024

There is an AssertionError failure caused by my TypeOfNode changes.

The issue is deeper. My changes just discovered an error in EnsoFile bultin. There should be no String objects floating thru the Enso interpreter, just Text wrappers around them. The EnsoFile case fixed in 0e4f631

@GregoryTravis GregoryTravis merged commit ff562cb into develop Mar 30, 2024
41 checks passed
@GregoryTravis GregoryTravis deleted the wip/gmt/8982-conversion-int-float branch March 30, 2024 14:42
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.

5 participants