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

Using conversions in a few places #7859

Merged
merged 26 commits into from
Oct 2, 2023
Merged

Using conversions in a few places #7859

merged 26 commits into from
Oct 2, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Sep 20, 2023

Pull Request Description

  • Shuffle a few froms into correct places:
    • Day_Of_Week.from removing Day_Of_Week_From module.
  • Adding short cut for http and https in Data.read so it calls onto Data.fetch giving a single entry point.
  • Moved URI extensions from Standard.Base.Data module into Standard.Base.Network.Extensions.
    • Added post extension for URI.
  • Added contains_key to JS_Object.
  • Restored into in JS_Object:
    • Follows old logic populating a constructor.
    • Will use conversion from JS_Object if present.
  • Added automatic deserialization of Date, Time_Of_Day and Date_Time from JSON.
    • Uses conversion from JS_Object.
  • Added conversion from Text to a HTTP_Method and type checking where HTTP_Method used in public APIs.
  • Added support for Date, Time_Of_Day and Date_Time in Table.from_objects.
  • Added expand_column to Table to expand JS_Object to values.
  • Add type checking for Table in right arguments (allowing Columns to be used).
  • Use type checking in Table.set to allow for conversion to a Column.
  • Remove some unused imports.
  • Fix for bug in S3 edge case.

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 force-pushed the wip/jd/use-conversions branch 4 times, most recently from e52972c to aee7296 Compare September 27, 2023 10:01
@jdunkerley jdunkerley marked this pull request as ready for review September 29, 2023 15:09
Comment on lines 21 to 23
@column Widget_Helpers.make_column_name_selector
Table.expand_column : Text | Integer -> Vector | Nothing -> Text | Table -> Table ! Type_Error
Table.expand_column self column fields=Nothing prefix=Nothing =
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an extension method and not just a regular method inside of Table.enso?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only because the from_objects would make the two files so intertwined felt more natural to have it next to here.

When we refactor from_objects, probably makes sense to fold in at that point.

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 generally, just a few small comments.

Tests look great, assuming we'll add more expand_column in the future, but I know that this is the plan 🙂

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Oct 2, 2023
@mergify mergify bot merged commit fb50eb7 into develop Oct 2, 2023
26 of 27 checks passed
@mergify mergify bot deleted the wip/jd/use-conversions branch October 2, 2023 14:54
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