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

Improving widgets and other minor tweaks. #7052

Merged
merged 9 commits into from Jun 19, 2023
Merged

Improving widgets and other minor tweaks. #7052

merged 9 commits into from Jun 19, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jun 16, 2023

Pull Request Description

  • Removed module argument from enso_project (new Project_Description.new API).
  • Removed the custom option from date and time parse/format dropdowns.
  • The format dropdown uses the value to create the dropdown. (Screenshot below)
  • Removed StorageType coalescing rules and replaced them with simpler logic in ObjectStorage.
  • Update signature for add_row_number and add aliases.

Screenshots

image

2023-06-19_11-44-58.mp4

Important Notes

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.

@jdunkerley jdunkerley marked this pull request as ready for review June 19, 2023 16:32
@jdunkerley jdunkerley changed the title Improving widgets and S3 APIs Improving widgets and other minor tweaks. Jun 19, 2023
CHANGELOG.
@@ -121,12 +120,16 @@ import project.Network.HTTP.HTTP_Version.HTTP_Version
import project.Network.URI.URI
import project.Random

from project.Data.Json.Extensions import all
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the original point of the Extensions files to speed up compilation/startup? Does this remove that benefit?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? I don't understand your question. In these Extensions modules, we have extension methods, and we need to import and export all these methods. I have recently changed the form of these import and export statements as there was a clash of Extensions symbol.

Project_Description.enso_project_builtin module
enso_project : Project_Description ! Module_Not_In_Package_Error
enso_project =
Project_Description.enso_project_builtin Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Project_Description.enso_project_builtin Nothing
Project_Description.new Nothing

In case any further logic is added to new.

Copy link
Member Author

Choose a reason for hiding this comment

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

Has to be there as it moves one frame up to find where it was called from.

If we pass through new it then ends up saying where Standard.Base is.

@@ -1201,7 +1202,7 @@ type Table
problem is reported.
@group_by Widget_Helpers.make_column_name_vector_selector
@order_by Widget_Helpers.make_order_by_selector
add_row_number : Text -> Integer -> Integer -> Vector (Text | Integer | Column_Selector) | Text | Integer | Column_Selector -> Vector (Text | Sort_Column) | Text | Sort_Column -> Problem_Behavior -> Table
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 removed? They are valid inputs IIRC and the widget should be based on the annotation anyway.

Is it due to a bug in dropdowns logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be but isn't yet - should be fixed one day but need to work around for now.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a ticket for it so that we can keep track of it?

IMO it may be worth to add a TODO note near the signature - as I assume once the bug is fixed, we should fixe the signature to allow this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

No need for a todo as I don't see us needing to revert this change.

I'll raise the drop-down issues as a ticket.

Comment on lines +13 to +17
if (profiles == null) {
var provider = ProfileFileSupplier.defaultSupplier();
var profileFile = provider.get();
profiles = profileFile.profiles().keySet().toArray(new String[0]);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to be annoying, because it's exactly what I previously suggested.

But after even further though, shouldn't we reload the profiles on every call? Or have some way to reload them? What if someone adds a profile while Enso is running? Currently it seems that they would have to restart it for the changes to take effect, are we fine with that? I guess this may be OK, just asking to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if I can do anything in the next S3 work.

Generally a minor thing but if I can detect a change using AWS API will do.

Comment on lines +65 to +67
if (currentType instanceof AnyObjectType) {
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

I like the early quit - for many storages it will mean that computing this will finish very quickly.

TestMessages.update(contextId, x1Id, ConstantsGen.NOTHING_BUILTIN),
TestMessages.update(contextId, mainRes1Id, ConstantsGen.NOTHING_BUILTIN),
TestMessages.update(contextId, x1Id, ConstantsGen.NOTHING),
TestMessages.update(contextId, mainRes1Id, ConstantsGen.NOTHING),
Copy link
Contributor

Choose a reason for hiding this comment

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

@jdunkerley this change is definitely not obvious.

Nothing is a builtin type, and they have special handling in the compiler. When the program is compiled

  1. All the builtins are loaded into the scope, making the type of Nothing Standard.Builtins.Main.Nothing
  2. When the compiler encounters the Nothing definition in the standard library
    @Builtin_Type
    type Nothing
    It shadows the builtin definition of Nothing with Standard.Base.Nothing.Nothing (the module where the new definition is)

Now returning to the test,

      from Standard.Base.Errors.Common import all
      main =
          x = IO.println "MyError"
          x

The program imports Standard.Base.Errors.Common which in turn imports project.Meta, and this PR changes Meta.enso test/micro-distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso and adds

+from Standard.Base.Nothing import Nothing
+

Now the compiler sees the definition of Nothing in the standard library and shadows the builtin definition. And that's why the return type of Nothing changes in the test.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jun 19, 2023
@mergify mergify bot merged commit 1859ccb into develop Jun 19, 2023
24 of 26 checks passed
@mergify mergify bot deleted the wip/jd/s3-apis branch June 19, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants