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

Remove Column type from Aggregate_Column, simplify Column_Selector, some new File_Formats #5646

Merged
merged 27 commits into from
Feb 16, 2023
Merged

Remove Column type from Aggregate_Column, simplify Column_Selector, some new File_Formats #5646

merged 27 commits into from
Feb 16, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Feb 13, 2023 •
edited
Loading

Pull Request Description

  • Updated Widget.Vector_Editor ready for use by IDE team.
  • Added get to Row to make API more aligned.
  • Added first_column, second_column and last_column to Table APIs.
  • Adjusted Column_Selector and associated methods to have simpler API.
  • Removed Column from Aggregate_Column constructors.
  • Added new Excel_Workbook type and added to Excel_Section.
  • Added new SQLiteFormatSPI and SQLite_Format.
  • Added new IamgeFormatSPI and Image_Format.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@jdunkerley jdunkerley force-pushed the wip/jd/5153-review-by_column-in-selectors-and-using-column-within-apis branch from 0c14e2a to b78f6fd Compare February 13, 2023 14:36
@jdunkerley jdunkerley changed the title Wip/jd/5153 review by column in selectors and using column within apis Remove Column type from Aggregate_Column, simplify Column_Selector, some new File_Formats Feb 13, 2023
@jdunkerley jdunkerley force-pushed the wip/jd/5153-review-by_column-in-selectors-and-using-column-within-apis branch from 85e4a78 to ae6b4aa Compare February 14, 2023 10:46
@jdunkerley jdunkerley force-pushed the wip/jd/5153-review-by_column-in-selectors-and-using-column-within-apis branch from f95cab1 to a6dfe67 Compare February 14, 2023 21:43
@jdunkerley jdunkerley marked this pull request as ready for review February 15, 2023 18:32
@jdunkerley
Fix Excel write.
dd1ae00 
Changelog.
@jdunkerley jdunkerley linked an issue Feb 15, 2023 that may be closed by this pull request
Review By_Column in selectors and using Column within APIs #5153
Closed
@jdunkerley jdunkerley added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 16, 2023
hubertp
hubertp approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@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.

nits

build.sbt
@@ -2092,6 +2093,7 @@ lazy val `std-image` = project
result
}.value
)
.dependsOn(`std-base` % "provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of weird that the classifier is here but I can see we do the same with std-table. Will need to investigate separately

distribution/lib/Standard/Image/0.0.0-dev/src/Image_File_Format.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Image/0.0.0-dev/src/Image_File_Format.enso Outdated Show resolved Hide resolved
@hubertp
Workaround doc parser bug with double brackets
7305d1c 
Doc parser does not seem to accept nested square brackets. In this case
they are spurious but the bug should be fixed in the rust parser.
@hubertp hubertp mentioned this pull request Feb 16, 2023
Open
@jdunkerley jdunkerley removed the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 16, 2023
jdunkerley and others added 2 commits February 16, 2023 12:01
@jdunkerley @radeusgd
Apply suggestions from code review
81d4367 
Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@jdunkerley
PR comments.
be6ee1a
jdunkerley
jdunkerley commented Feb 16, 2023
View reviewed changes
Copy link
Member Author

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Fix comments.

radeusgd
radeusgd approved these changes Feb 16, 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.

Looks like really great improvements 🌮 🎉

Added some comments where I think we could clarify some docs, ~1 edge case (the 256 sheets in excel) and a few tests, but overall it looks all good.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Feb 16, 2023
@mergify mergify bot merged commit 1bc2750 into develop Feb 16, 2023
@mergify mergify bot deleted the wip/jd/5153-review-by_column-in-selectors-and-using-column-within-apis branch February 16, 2023 15:15
@jdunkerley jdunkerley linked an issue Feb 16, 2023 that may be closed by this pull request
Implement Excel_Files and Excel_Connection #5263
Closed
@jdunkerley jdunkerley mentioned this pull request Feb 20, 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

@hubertp hubertp hubertp approved these changes

@GregoryTravis GregoryTravis GregoryTravis approved these changes

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

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach JaroslavTulach 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.

Implement Excel_Files and Excel_Connection Review By_Column in selectors and using Column within APIs
4 participants
@jdunkerley @hubertp @radeusgd @GregoryTravis