Sort output of S3_File.list and Enso_File.list
#11929
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,7 +187,7 @@ type S3_File | |
| S3_File.Value (S3_Path.Value bucket key) self.credentials | ||
| files = pair.second . map key-> | ||
| S3_File.Value (S3_Path.Value bucket key) self.credentials | ||
| (sub_folders + files) . sort | ||
|
|
||
| ## ALIAS load bytes, open bytes | ||
| ICON data_input | ||
|
|
@@ -616,3 +616,12 @@ translate_file_errors related_file result = | |
| s3_path = S3_Path.Value error.bucket error.key | ||
| s3_file = S3_File.Value s3_path related_file.credentials | ||
| Error.throw (File_Error.Not_Found s3_file) | ||
|
|
||
| ## PRIVATE | ||
| type S3_File_Comparator | ||
|
There was a problem hiding this comment. This type should probably be marked as |
||
| compare x y = Ordering.compare x.s3_path y.s3_path | ||
|
|
||
| hash x = S3_File_Comparator.hash_builtin x | ||
| hash_builtin x = @Builtin_Method "Default_Comparator.hash_builtin" | ||
|
|
||
| Comparable.from that:S3_File = Comparable.new that S3_File_Comparator | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| import project.Any.Any | ||
| import project.Data.Color.Color | ||
| import project.Data.Json.JS_Object | ||
| import project.Data.Ordering.Comparable | ||
| import project.Data.Ordering.Ordering | ||
| import project.Data.Numbers.Integer | ||
| import project.Data.Text.Encoding.Encoding | ||
| import project.Data.Text.Text | ||
|
|
@@ -444,10 +446,11 @@ type Enso_File | |
| if self.is_directory.not then Error.throw (Illegal_Argument.Error "Cannot `list` a non-directory.") else | ||
| # Remove secrets from the list - they are handled separately in `Enso_Secret.list`. | ||
| assets = list_assets self . filter f-> f.asset_type != Enso_Asset_Type.Secret | ||
| results = assets.map asset-> | ||
| file = Enso_File.Value (self.enso_path.resolve asset.name) | ||
| Asset_Cache.update file asset | ||
| file | ||
| results.sort | ||
|
|
||
| ## GROUP Output | ||
| ICON folder_add | ||
|
|
@@ -583,3 +586,12 @@ File_Like.from (that : Enso_File) = File_Like.Value that | |
| ## PRIVATE | ||
| Writable_File.from (that : Enso_File) = if Data_Link.is_data_link that then Data_Link_Helpers.interpret_data_link_as_writable_file that else | ||
| Writable_File.Value that Enso_File_Write_Strategy.instance | ||
|
|
||
| ## PRIVATE | ||
| type Enso_File_Comparator | ||
| compare x y = Ordering.compare x.enso_path y.enso_path | ||
|
|
||
| hash x = Enso_File_Comparator.hash_builtin x | ||
| hash_builtin x = @Builtin_Method "Default_Comparator.hash_builtin" | ||
|
|
||
| Comparable.from that:Enso_File = Comparable.new that Enso_File_Comparator | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,16 @@ | ||
| private | ||
|
|
||
| import project.Data.Ordering.Comparable | ||
| import project.Data.Ordering.Ordering | ||
| import project.Data.Ordering.Vector_Lexicographic_Order | ||
| import project.Data.Text.Text | ||
| import project.Data.Vector.Vector | ||
| import project.Enso_Cloud.Enso_File.Enso_File | ||
| import project.Enso_Cloud.Enso_User.Enso_User | ||
| import project.Error.Error | ||
| import project.Errors.Illegal_Argument.Illegal_Argument | ||
| import project.Errors.Unimplemented.Unimplemented | ||
| import project.Internal.Ordering_Helpers.Default_Comparator | ||
| import project.Internal.Path_Helpers | ||
| import project.Nothing.Nothing | ||
| from project.Data.Boolean import Boolean, False, True | ||
|
|
@@ -72,3 +76,12 @@ normalize segments = | |
| ## We need to call normalize again, because technically a path `enso://a/../~/../../~` is a valid path | ||
| that points to the user home and it should be correctly normalized, but requires numerous passes to do so. | ||
| @Tail_Call normalize new_segments | ||
|
|
||
| ## PRIVATE | ||
| type Enso_Path_Comparator | ||
| compare x y = Vector_Lexicographic_Order.compare x.path_segments y.path_segments | ||
|
|
||
| hash x = Enso_Path_Comparator.hash_builtin x | ||
| hash_builtin x = @Builtin_Method "Default_Comparator.hash_builtin" | ||
|
|
||
| Comparable.from that:Enso_Path = Comparable.new that Enso_Path_Comparator | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,6 +261,13 @@ add_specs suite_builder = | |
| r3.should_be_a Vector | ||
| r3.map .name . should_contain object_name | ||
|
|
||
| group_builder.specify "list should sort its output" <| | ||
| r = root.list | ||
| r.should_be_a Vector | ||
| r . should_equal (r.sort on=(x-> x.path)) | ||
| # Check sorting directly. | ||
| r . reverse . sort . should_equal r | ||
|
|
||
| group_builder.specify "will fail if no credentials are provided and no Default credentials are available" pending=(if AWS_Credential.is_default_credential_available then "Default AWS credentials are defined in the environment and this test has no way of overriding them, so it is impossible to test this scenario in such environment.") <| | ||
| root_without_credentials = S3_File.new "s3://"+bucket_name+"/" | ||
| r = root_without_credentials.list | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,13 @@ add_specs suite_builder setup:Cloud_Tests_Setup = suite_builder.group "Enso Clou | |
| Data.list test_root.get . map .name . should_contain "test_file.json" | ||
| Data.list test_root.get.path . map .name . should_contain "test_file.json" | ||
|
|
||
| group_builder.specify "list should sort its output" <| | ||
| r = Enso_File.home.list | ||
| r.should_be_a Vector | ||
| r . should_equal (r.sort on=.path) | ||
| # Check sorting directly. | ||
| r . reverse . sort . should_equal r | ||
|
|
||
| group_builder.specify "should allow to create and delete a directory" <| | ||
| my_name = "my_test_dir-" + (Random.uuid.take 5) | ||
| my_dir = (test_root.get / my_name).create_directory | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the linter's benefit please tag
## PRIVATE.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. why does the
S3_Path_Comparatorcontain a methodhash_builtin? It does not seem right.I'm not sure we have a rule for this, but every builtin should have at most one (or perhaps, exactly one) registration.
Instead, I think we should just delegate to default comparator there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry the suggestion I gave you above won't work because
Default_Comparatoris private.Still registering a duplicated builtin to get around private check is not right. Moreover there are movements (#6282) to allow builtins only from the
Baselibrary, so this would not work then.I guess the correct solution then may be to delegate to the default comparator by computing the hash of the element:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved just the hash builtin to Ordering.enso:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it would be more efficient to call the builtin directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That still does not prepare us for #6282. Once that issue is addressed, builtin methods cannot be exposed from Base directly. You would need to create a method:
that would be wrapping the builtin, then you could export
default_comparator_hashas part of public API.Still, I'm not quite sure if this is the correct approach.
I think the correct approach to creating a custom hash function is to rely on
Comparable.fromon the elements of the custom type and then combining the resulting hashes (if there's >1 element). We currently perhaps lack a generic function to combine hashes - that's perhaps something that could be added to the public API of Base. We could just compute the hash of the array of hashes to combine, I think that should work okay.On a hot path the overhead should be totally negligible. But I guess that would be a question to the engine team. Regardless, the guidelines from the engine team were to not export builtin methods as public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For implementing a generic hash function, I would prefer to use the existing implementations in
HashCodeNoderather than create another, different generic one in Enso.What I am currently doing:
S3_File.enso:
Ordering.enso:
This results in a stack overflow.
@JaroslavTulach should I try to use
HashCodeNodefor a generic hash implementation?