Tidy Standard.Base part 5 of n ... (hopefully the end...) #3929

jdunkerley · 2022-11-29T13:44:17Z

Pull Request Description

Moved Any, Error and Panic to Standard.Base.
Separated Json and Range extensions into own modules.
Tidied Case, Case_Sensitivity, Encoding, Matching, Regex_Matcher, Span, Text_Matcher, Text_Ordering and Text_Sub_Range in Standard.Base.Data.Text.
Tidied Standard.Base.Data.Text.Extensions and stopped it re-exporting anything.
Tidied Regex_Mode. Renamed Option to Regex_Option and added type to export.
Tidied up Regex space.
Tidied up Meta space.
Remove Matching from export.
Moved Standard.Base.Data.Boolean to Standard.Base.Boolean.

Additional Work

Removed Standard.Table dependency on Standard.Visualization by moving default_visualization to be an extension method.
Removed remaining dubious constructor exports in Standard.Database.
Completed changes for Standard.Database in line with design in ticket.

Important Notes

Moved to_json and to_default_visualization_data from base types to extension methods.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

JaroslavTulach · 2022-12-02T07:30:07Z

distribution/lib/Standard/Base/0.0.0-dev/package.yaml

-          - Standard.Base.Network.Http.put_json
-          - Standard.Base.Network.Http.head
-          - Standard.Base.Network.Http.options
+          - Standard.Base.Network.HTTP.HTTP.new


These are the component groups visible in component browsers! Yeah, simplifying the Web part is probably good idea.

A start at tidying Web API. Will design and expand over next month or two.

distribution/lib/Standard/Base/0.0.0-dev/src/Any.enso

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

JaroslavTulach · 2022-12-02T07:36:57Z

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

+   > Example
+     Convert a vector to JSON.
+         [1, 2, 3, 4].to_json
+Any.to_json : Json


Moving these extension methods from Json.enso to Json/Extensions.enso helps with with imports somehow, right? How the imports looked and will look once this PR is integrated?

from Standard.Base import alll brings it in, and will be the general way for users.
Within Standard.Base or being explicit import Standard.Base.Data.Json.Extensions adds the to_json support in.

I imagine we will replace to_json with a Json.from method and use to Json. With the exception of Text we have less and less extension methods.

I really like that we are getting a more explicit way to import the extension methods (by importing this separate module). It feels a lot cleaner (although I appreciate auto-importing them in the from Standard.Base import all also makes sense).

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

JaroslavTulach · 2022-12-02T07:43:01Z

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

@@ -19,7 +19,7 @@ polyglot java import org.enso.base.ObjectComparator
     Otherwise can support a custom fallback comparator.
 new : Nothing | (Any -> Any -> Ordering) -> ObjectComparator
 new custom_comparator=Nothing =
-    comparator_to_java cmp x y = handle_incomparable_value (cmp x y . to_sign)
+    comparator_to_java cmp x y = Incomparable_Values_Error.handle_errors (cmp x y . to_sign)


I'd like to point out that various compare_to methods throw different Error. Shouldn't all compare failures result into the same Incomparable_Values_Error?

Yes that would be a fair step but out of scope for this PR.

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

test/Tests/src/Semantic/Meta_Spec.enso

hubertp

radeusgd · 2022-12-02T13:19:31Z

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

@@ -310,9 +312,9 @@ into_helper fmt json = case fmt of
    _ ->


This into_helper may need to be revisited in some future - I think the way it (ab)uses the atoms to create the formats will be violating our up-coming type system.

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

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

radeusgd · 2022-12-02T14:46:11Z

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

-# TODO Dubious constructor export
-from project.Data.Text.Matching.No_Matches_Found import all
-from project.Data.Text.Matching.No_Matches_Found export all
+from project.Data.Boolean import Boolean, True, False


Makes me think if we should have a Standard.Core covering the most primitive types, essentially replacing the micro-distribution.

Then in proper Standard.Base we could do from Standard.Core import all instead. And ofc we'd re-export Core in Base, so that regular users would just use Base.

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Network/Extensions.enso

…o Json extensions.

Tidy Regex_Mode.

radeusgd · 2022-12-02T15:13:34Z

distribution/lib/Standard/Base/0.0.0-dev/src/Network/URI.enso

    scheme : Text ! Nothing
-    scheme self = Internal.handle_nothing self.internal_uri.getScheme
+    scheme self = handle_nothing self.internal_uri.getScheme


This may not be for this PR, but IMO this API needs to be revisited.

I highly doubt what we want to do here is to throw Nothing. Apart from some control flow, I doubt we want to throw Nothing ever - once it is caught somewhere down the dataflow, it's impossible to tell what it originated from or what it means. It's also rather unclear for the user to see 'Nothing' was thrown. If it was a convention we used across all stdlib and users were used to it (it kind of used to be) then maybe it would be ok, but in just a few places, it is a source of confusion IMO.

IMO we should either throw some more meaningful error like Missing_Field or something. But ideally, I'd just return Nothing as a value instead, making the type not Text ! Nothing but Text | Nothing.

What do you think @jdunkerley ?

Also, I'm not saying to adapt it in this PR (let's merge it ASAP), but I think may be worth noting so that we revisit it in the near future.

radeusgd · 2022-12-02T15:18:27Z

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso

+    ## PRIVATE
+
+       Utility method for running an action with Java exceptions mapping.
+    handle_java_exceptions file ~action =
+        Panic.catch IOException action caught_panic->
+            File_Error.wrap_io_exception file caught_panic.payload.cause
+
+    ## PRIVATE
+
+       Converts a Java `IOException` into its Enso counterpart.
+    wrap_io_exception file io_exception =
+        if io_exception.is_a NoSuchFileException then Error.throw (File_Error.Not_Found file) else
+            if io_exception.is_a FileAlreadyExistsException then Error.throw (File_Error.Already_Exists file) else
+                if io_exception.is_a AccessDeniedException then Error.throw (File_Error.IO_Error file "You do not have permission to access the file") else
+                    Error.throw (File_Error.IO_Error file "An IO error has occurred: "+io_exception.to_text)


With all that logic, this file is growing pretty fast. It's already quite large.

Maybe we should not keep all basic errors in a single file. What about sorting them into smaller modules 'thematically' and then possibly re-exporting through Common for easier importing?

I think it would be more manageable, as such big files are a bit of a pain.

radeusgd · 2022-12-02T15:20:42Z

distribution/lib/Standard/Base/0.0.0-dev/src/System/Process/Exit_Code.enso

       > Example
         Convert a success code to a corresponding number.

-             import Standard.Base.System.Process.Exit_Code
+             import Standard.Base.System.Process.Exit_Code.Exit_Code

-             example_to_number = Exit_Code.Exit_Success.to_number
+             example_to_number = Exit_Code.Success.to_number


Makes me remember that we used to have plans to have an automatic compilation of Examples.

I think it is something worth considering soon-ish so that we can assure that the examples provided in the docs are correct.

IMO likely this may not take much more time than manually making sure all of them work - and before an official release we definitely should make sure the examples are correct - giving incorrect examples to the users is going to be extremely annoying for them. And given so many refactors I'm almost sure there are other examples which are broken, however careful we would be it's hard to keep track of all.

agree need to validate examples - yes picking them out and building them automagically seems right to me.

radeusgd · 2022-12-02T15:33:28Z

distribution/lib/Standard/Base/0.0.0-dev/src/Warning.enso

@@ -106,7 +106,7 @@ type Warning
       > Example
         Detach warnings of a specific type.

-             result = Warning.detach_selected_warnings value (_.is_a Illegal_State_Error)
+             result = Warning.detach_selected_warnings value (_.is_a Illegal_State)


Suggested change

result = Warning.detach_selected_warnings value (_.is_a Illegal_State)

result = Warning.detach_selected_warnings value (_.is_a Illegal_State.Error)

While I'd expect is_a to work the way you expect too, I don't think that is the case currently.

I run an example check:

from Standard.Base import all type Foo Bar x main = y = Foo.Bar 42 IO.println <| y.is_a Foo IO.println <| y.is_a Foo.Bar

and it gives back

False True

We'd probably both expect both to be True, but currently that is not the case.

I think we need to revisit how is_a works, along with how Panic.catch, Error.catch and Panic.recover filtering also works.

cc: @hubertp @JaroslavTulach as I'm not sure who would be scheduling the fixes to these?

This was broken before Illegal_State_Error is the type as well. I agree we should review is_a and catch.
Ideally would be able to pass both as you say.

distribution/lib/Standard/Base/0.0.0-dev/src/Panic.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Postgres/Postgres_Connection.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Main.enso

radeusgd · 2022-12-02T15:45:05Z

distribution/lib/Standard/Searcher/0.0.0-dev/src/Main.enso

@@ -52,5 +52,5 @@ number_input = Error.unimplemented "This function should not be called."
             column_1 = ["name", "Enso"]
             column_2 = ["stars", 5000]
             table = Table.new [column_1, column_2]
-table_input = Error.unimplemented "This function should not be called."
+table_input = Unimplemented.throw "This function should not be called."


Only now noticed it. I understand that it is supposed to allow easy access to the examples.

But will the component browser somehow know to not insert these fake functions into nodes? I guess it will just insert them. Won't this be weird and confusing?

distribution/lib/Standard/Table/0.0.0-dev/src/Excel/Excel_Writer.enso

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Table_Helpers.enso

distribution/lib/Standard/Visualization/0.0.0-dev/src/Helpers.enso

distribution/lib/Standard/Visualization/0.0.0-dev/src/SQL/Visualization.enso

radeusgd · 2022-12-02T16:25:38Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Regex/Engine/Default.enso

-# TODO Dubious constructor export
-from project.Data.Text.Regex.Engine.Default.Option import all
-from project.Data.Text.Regex.Engine.Default.Option export all
-
 ## Options specific to the `Default` regular expression engine.
 type Option


Do you think maybe it would be worth re-exporting the global Regex_Options in this Option type? So that we could mark that all options - both engine specific and global can be accessed through it?

(not sure though as it may cause confusion that the global ones are of different type, so there are pros and cons to this)

The Regex design is not the final one - we are going to redo it in due course.
Hopefully the lower level engine will never be exposed to end users!

test/Visualization_Tests/src/Table_Spec.enso

test/micro-distribution/lib/Standard/Base/0.0.0-dev/src/Error.enso

radeusgd

Awesome!

jdunkerley changed the title ~~Tidy Standard.Base part 4 of n ...~~ Tidy Standard.Base part 5 of n ... (hopefully the end...) Nov 29, 2022

jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 29, 2022

jdunkerley force-pushed the wip/jd/base-tidy-5 branch 4 times, most recently from 46aac45 to 8f3dbd7 Compare December 1, 2022 17:29

jdunkerley marked this pull request as ready for review December 1, 2022 18:18

jdunkerley requested review from radeusgd, 4e6, JaroslavTulach and hubertp as code owners December 1, 2022 18:18

jdunkerley force-pushed the wip/jd/base-tidy-5 branch from 14b25ed to 8a707de Compare December 1, 2022 19:14

JaroslavTulach approved these changes Dec 2, 2022

View reviewed changes

jdunkerley force-pushed the wip/jd/base-tidy-5 branch from 65c3a47 to 663e302 Compare December 2, 2022 08:23

hubertp approved these changes Dec 2, 2022

View reviewed changes

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Json/Extensions.enso Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Network/Extensions.enso Outdated Show resolved Hide resolved

jdunkerley added 9 commits December 2, 2022 15:12

Move Error and Panic to Standard.Base.

a8104e2

Move Any, Error and Panic to Standard.Base.

e6d642d

Move Json extensions to own module to make import more explicit.

054b837

Missing import

5b23c29

Move to_json and to_default_visualisation_data from primitive types t…

66457ff

…o Json extensions.

Separate up_to and down_to to own module.

ad32c79

Tidy Case, Case_Sensitivity and Encoding.

9e27125

Tidy Matching and remove from export.

2f05572

Tidy Extensions and Span.

862d8be

Tidy Regex_Mode.

jdunkerley added 7 commits December 2, 2022 15:12

Fix issues.

52362f7

Parser test works now.

a741938

Remove some method imports.

233f4b4

Rename Meta _Data constructors.

d484238

Fix import issue.

d261b9f

Fit HTTP test issues.

a1f7343

Fix issue.

d2f1541

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Panic.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Postgres/Postgres_Connection.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Database/0.0.0-dev/src/Main.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Excel/Excel_Writer.enso Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Table_Helpers.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Table_Helpers.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Visualization/0.0.0-dev/src/Helpers.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

distribution/lib/Standard/Visualization/0.0.0-dev/src/SQL/Visualization.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

test/Visualization_Tests/src/Table_Spec.enso Show resolved Hide resolved

radeusgd reviewed Dec 2, 2022

View reviewed changes

test/micro-distribution/lib/Standard/Base/0.0.0-dev/src/Error.enso Outdated Show resolved Hide resolved

radeusgd approved these changes Dec 2, 2022

View reviewed changes

PR comments.

5308982

jdunkerley force-pushed the wip/jd/base-tidy-5 branch from d49b224 to 5308982 Compare December 2, 2022 17:33

jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Dec 2, 2022

mergify bot merged commit 0ad70c6 into develop Dec 2, 2022

mergify bot deleted the wip/jd/base-tidy-5 branch December 2, 2022 18:08

wdanilo mentioned this pull request Feb 6, 2023

Revisit Decimal.parse and Integer.parse and convert to builtins #4734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy Standard.Base part 5 of n ... (hopefully the end...) #3929

Tidy Standard.Base part 5 of n ... (hopefully the end...) #3929

jdunkerley commented Nov 29, 2022 •

edited

Loading

JaroslavTulach Dec 2, 2022

jdunkerley Dec 2, 2022

JaroslavTulach Dec 2, 2022

jdunkerley Dec 2, 2022

radeusgd Dec 2, 2022

JaroslavTulach Dec 2, 2022

jdunkerley Dec 2, 2022

hubertp left a comment

radeusgd Dec 2, 2022

radeusgd Dec 2, 2022

radeusgd Dec 2, 2022

radeusgd Dec 2, 2022

radeusgd Dec 2, 2022

jdunkerley Dec 2, 2022

radeusgd Dec 2, 2022

jdunkerley Dec 2, 2022

radeusgd Dec 2, 2022

radeusgd Dec 2, 2022

jdunkerley Dec 2, 2022

radeusgd left a comment

	result = Warning.detach_selected_warnings value (_.is_a Illegal_State)
	result = Warning.detach_selected_warnings value (_.is_a Illegal_State.Error)

Tidy Standard.Base part 5 of n ... (hopefully the end...) #3929

Tidy Standard.Base part 5 of n ... (hopefully the end...) #3929

Conversation

jdunkerley commented Nov 29, 2022 • edited Loading

Pull Request Description

Additional Work

Important Notes

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

hubertp left a 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

radeusgd left a comment

Choose a reason for hiding this comment

jdunkerley commented Nov 29, 2022 •

edited

Loading