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

Widget support for Data.fetch and Data.post. #11199

Merged
merged 10 commits into from
Sep 27, 2024
Merged

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Sep 27, 2024

Pull Request Description

  • Add pretty for Date and Time.
  • Add constructors for JS_Object and Dictionary to the component browser.
  • Add widgets to Dictionary methods.
    image
  • Add conversion from Vector to Dictionary.
  • Add pair method shorthand for Pair.Value.
  • Created widget for Header.
  • Added widgets for Data.fetch and Data.post.
  • Added widgets for Request_Body constructors.
  • Update the Forbidden Operation message to be friendlier.
    image

Video before fixing Forbidden Message:

2024-09-27_10-44-26.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,
    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 jdunkerley marked this pull request as ready for review September 27, 2024 13:27
@jdunkerley jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 27, 2024
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Approving engine changes

@@ -16,7 +16,7 @@ instance =
## PRIVATE
create_dry_run_file file copy_original =
_ = [file, copy_original]
Error.throw (Forbidden_Operation.Error "Currently dry-run is not supported for S3_File, so writing to an S3_File is forbidden if the Output context is disabled.")
Error.throw (Forbidden_Operation.Error "As writing is disabled, no action has been performed. Press the Write button ▶ to write the file.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a helper in Context since it's repeated so much.

@@ -32,14 +38,12 @@ from project.Data.Text.Extensions import all
to pass a foreign map into Enso, where it will be treated as a Dictionary.
@Builtin_Type
type Dictionary key value
## PRIVATE
ADVANCED
## ICON array_new2
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: Why is the ICON called array_new2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just the name in figma, there are a few "array_new" variants.

@@ -423,3 +439,19 @@ type Dictionary key value
## PRIVATE
get_builtin : Any -> Any -> Any
get_builtin self key ~if_missing = @Builtin_Method "Dictionary.get_builtin"

## PRIVATE
key_widget dict:Dictionary -> Widget =
Copy link
Member

Choose a reason for hiding this comment

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

Can these methods be moved into their own private helper module?

Copy link
Member Author

Choose a reason for hiding this comment

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

We often need to get the widgets in other functions so tend to keep them as ## PRIVATE methods in the module.

insert : Any -> Any -> Dictionary
insert self key value = @Builtin_Method "Dictionary.insert"
insert self key=(Missing_Argument.throw "key") value=(Missing_Argument.throw "value") =
self.insert_builtin key value
Copy link
Member

Choose a reason for hiding this comment

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

Is the insert_builtin method defined nowhere? Autoregistered? I'd make it static, put it into a private helper module.

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's auto-registered the same as the "remove_builtin" - we should do a review of what we need to do with the builtins to make it the best set up whenever suits you.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -295,3 +295,12 @@ check_start_valid start function max=3 =
used_start = if start < 0 then start + 2 else start
if used_start < 0 || used_start >= max then Error.throw (Index_Out_Of_Bounds.Error start max) else
function used_start

## ICON array_new
Copy link
Member

Choose a reason for hiding this comment

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

ICON array_new? What's the difference to array_new2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Slightly different look...
image

## PRIVATE
Convert to a Enso code representation of this Date.
pretty : Text
pretty self = "(Date.new " + self.year.to_text + " " + self.month.to_text + " " + self.day.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.

Eh! Another way to persist a value.

Wouldn't it be better to have type Code and then conversions to instances of the Code type? Because right now you get a code, but you may not have the right imports! Code could carry both - the imports and the code to construct the instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is matching our current pretty methods which work for some types, we can happily review and work out what we should do longer term.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Sep 27, 2024
@mergify mergify bot merged commit 28bbc34 into develop Sep 27, 2024
42 checks passed
@mergify mergify bot deleted the wip/jd/make-post-easier branch September 27, 2024 18:08
jdunkerley added a commit that referenced this pull request Oct 7, 2024
- Add `pretty` for `Date` and `Time`.
- Add constructors for `JS_Object` and `Dictionary` to the component browser.
- Add widgets to `Dictionary` methods.
![image](https://github.com/user-attachments/assets/4f6c58d5-9eb5-40e5-96c1-2e06e23051d0)
- Add conversion from Vector to Dictionary.
- Add `pair` method shorthand for `Pair.Value`.
- Created widget for `Header`.
- Added widgets for `Data.fetch` and `Data.post`.
- Added widgets for `Request_Body` constructors.
- Update the Forbidden Operation message to be friendlier.
![image](https://github.com/user-attachments/assets/eaac5def-a91f-450f-b814-d776311962e3)

Video before fixing Forbidden Message:

https://github.com/user-attachments/assets/f9c4bde4-3f0a-49f1-a3ca-a0aaa3219286

(cherry picked from commit 28bbc34)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants