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.

Sign up for GitHub

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

Jump to bottom

First part of AWS S3 API, various small fixes. #6973

Merged
merged 34 commits into from
Jun 15, 2023
Merged

First part of AWS S3 API, various small fixes. #6973

merged 34 commits into from
Jun 15, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jun 7, 2023 •
edited
Loading

Pull Request Description

  • Add type detection for Mixed columns when calling column functions.
  • Excel uses column name for missing headers.
  • Add aliases for parse functions on text.
  • Adjust Date, Time_Of_Day and Date_Time parse functions to not take Nothing anymore and provide dropdowns.
    • Removed built-in parses.
    • All support Locale.
    • Add support for missing day or year for parsing a Date.
    • All will trim values automatically.
  • Added ability to list AWS profiles.
  • Added ability to list S3 buckets.
  • Workaround for Table.aggregate so default item added works.

Screenshots

image

image

image

image

image

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/aws-api branch 4 times, most recently from 15fb641 to e483321 Compare June 9, 2023 21:38
jdunkerley and others added 23 commits June 14, 2023 14:17
@jdunkerley
Separate AWS_Credential from Redshift.
17bc8f6
@jdunkerley
Separate AWS_Credential from Redshift.
66436f1
@jdunkerley
Add AWS v2 SDK.
ce48901 
Ability to get profiles and create provider.
Initial S3 functionality - list buckets.
@jdunkerley
Fix AWS widget.
b2dd005
@jdunkerley
When a missing column name in Excel use the Excel Column Name
62086bc
@jdunkerley
First attempt at resolving Mixed value types.
ef81957
@jdunkerley
No need for MSF to do anything - just ask underlying.
ae65899 
Put default implementation on Storage itself.
Move computation to ObjectStorage.
@jdunkerley
No need for this override.
320d31b
@jdunkerley
Use NumericConverter.
3a2681c
@jdunkerley
Drop down for date formats.
7ae7356
@jdunkerley
Tweak type signatures.
b671d76
@jdunkerley
Use a static cache for profile names so only called once.
0f3781e
@jdunkerley
Add ALIAS for to_* for the parse functions.
8d7cbe5 
Removed `parse_uri` as same as `to_uri`.
@jdunkerley
Missing file commit.
66448b0
@jdunkerley
Using same date parse across stack.
3864baa 
Supports missing day or missing year.
Uses US Locale.
@jdunkerley
Remove builtin parse for Date and Date_Time.
141327b 
Support Locale in Date parse.
@jdunkerley
Remove builtin parse for Time_Of_Day.
5d33ea8 
Align API with other parse methods.
@jdunkerley
Adding more drop downs for date time formats.
c6c870b
@jdunkerley
Update test.
ab948bc
@jdunkerley
Attempt to get legal review to pass...
8ae2128 
Delete html file.
@radeusgd @jdunkerley
Updating legal review
8965004
@jdunkerley
Merge
d61bc66
@jdunkerley
PR comment.
7d686de 
Dropdown for Date_Time formats.
Fix issue with get_annotation for Date_Time.
radeusgd
radeusgd reviewed Jun 15, 2023
View reviewed changes
distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso Outdated
@@ -62,11 +62,10 @@ type File

example_new = File.new Examples.csv_path
new : (Text | File) -> File
new path =
new path:(Text|File) =
Copy link
Member

Choose a reason for hiding this comment

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

We should probably discuss about the best practices.

For me, the preferred syntax in cases like this would be

Suggested change
new path:(Text|File) =
new (path : Text|File) =

IMO it is more readable, it is closer to languages like Scala. But it is just a personal preference, I'm open to discussion. But I think ideally we should agree on one convention and try to use it consistently as a team.

I think the (x : A) one is used a bit more commonly throughout the codebase, that's why I'm nitpicking here.

radeusgd
radeusgd reviewed Jun 15, 2023
View reviewed changes
tools/legal-review-helper/package-lock.json
@@ -0,0 +1,645 @@
{
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 not sure if we should commit the package lock for this tool.

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 not sure, toplevel has package-lock committed. I guess there's no harm in it being here too. If it's a problem we can remove it.

radeusgd
radeusgd approved these changes Jun 15, 2023
View reviewed changes
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.

Generally looks good.

Some comments with suggestions in line.

I'm worried about the potential duplication of logic for unifying the types, I feel like it's quite easy to get it to be inconsistent. It's not a problem to be solved in this PR tho - but maybe worth chatting about it sometime.

@radeusgd
Copy link
Member

(btw. the examples next to formats for date/time are just pure gold! I think it's a very cool idea that may be worth expanding into other places)

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jun 15, 2023
@radeusgd
Copy link
Member

Ah, and as I was mentioning - I think we need to figure out some way to test S3 and related stuff on CI relatively ASAP - while I appreciate we don't want to make our default checks too slow, the nightly check and an opt-in label to run the additional checks on a specific PR would be really useful - otherwise it's just too easy to introduce breaking changes.

@radeusgd radeusgd removed the CI: Ready to merge This PR is eligible for automatic merge label Jun 15, 2023
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jun 15, 2023
@mergify mergify bot merged commit 760fb71 into develop Jun 15, 2023
@mergify mergify bot deleted the wip/jd/aws-api branch June 15, 2023 16:20
@enso-bot enso-bot bot mentioned this pull request Jun 16, 2023
Closed
Frizi pushed a commit that referenced this pull request Jun 19, 2023
@jdunkerley @Frizi
First part of AWS S3 API, various small fixes. (#6973)
83ad16c 
- Add type detection for `Mixed` columns when calling column functions.
- Excel uses column name for missing headers.
- Add aliases for parse functions on text.
- Adjust `Date`, `Time_Of_Day` and `Date_Time` parse functions to not take `Nothing` anymore and provide dropdowns.
- Removed built-in parses.
- All support Locale.
- Add support for missing day or year for parsing a Date.
- All will trim values automatically.
- Added ability to list AWS profiles.
- Added ability to list S3 buckets.
- Workaround for Table.aggregate so default item added works.
@radeusgd radeusgd mentioned this pull request Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@hubertp hubertp Awaiting requested review from hubertp hubertp is a code owner

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jdunkerley @radeusgd @GregoryTravis @JaroslavTulach