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

Minor improvements from last couple of Book Clubs #8034

Merged
merged 5 commits into from
Oct 12, 2023
Merged

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Oct 12, 2023

Pull Request Description

  • Added some ALIASes.
  • Added sheet to Excel_Workbook to give familiar API to read sheet.
  • Added conversion from range to vector allowing easy use with Zip.
  • Add `Map.from_keys_and_values.

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 added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 12, 2023
Comment on lines +203 to +205
sheet : Text | Integer -> Table
sheet self name:(Text|Integer) =
self.read_section (Excel_Section.Worksheet name 0 Nothing)
Copy link
Member

Choose a reason for hiding this comment

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

Suggesting an alternative name for this: get_sheet

IMHO it sounds a bit better here, but not strongly attached to it

Copy link
Member Author

Choose a reason for hiding this comment

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

sheet is consistent with the Excel VBA API.

Comment on lines +269 to +283
Test.specify "should allow building the map from two vectors" <|
expected = Map.empty . insert 0 0 . insert 3 -5 . insert 1 2
Map.from_keys_and_values [0, 3, 1] [0, -5, 2] . should_equal expected

Test.specify "should allow building the map from vector like things" <|
expected = Map.empty . insert 0 0 . insert 1 -5 . insert 2 2
Map.from_keys_and_values (0.up_to 3) [0, -5, 2] . should_equal expected

Test.specify "should not allow building with duplicate keys unless explicitly allowed" <|
expected = Map.empty . insert 0 0 . insert 3 -5 . insert 1 2
Map.from_keys_and_values [0, 3, 1, 0] [3, -5, 2, 0] . should_fail_with Illegal_Argument
Map.from_keys_and_values [0, 3, 1, 0] [3, -5, 2, 0] error_on_duplicates=False . should_equal expected

Test.specify "should not allow different length vectors when building" <|
Map.from_keys_and_values [0, 3, 1] [3, -5, 2, 0] . should_fail_with Illegal_Argument
Copy link
Member

Choose a reason for hiding this comment

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

IMHO it feels a bit weird to use this instead of Map.from_vector - the keys and values are kind of 'detached' from one another.

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 liked it when combined with the Range conversions, but yes no real reason you can't use zip then from_vector just makes it a one step process.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I'm not sure if I understand the use-case with Range conversions here. Could you please give an example?


Is this essentially needed because we still don't have the Map literal? Or is it a separate problem?

Because if it's the Map literal thing - maybe we could mark this as UNSTABLE and use for now but reserve the right to remove later once Map literal appears?

@@ -144,7 +144,8 @@ type Array
sort self (order = Sort_Direction.Ascending) on=Nothing by=Nothing on_incomparable=Problem_Behavior.Ignore =
Vector.sort self order on by on_incomparable

## GROUP Selections
## ALIAS first, last, slice, sample
GROUP Selections
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to .drop as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

thought about it but drop is inverse so didnt feel right

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Oct 12, 2023
@mergify mergify bot merged commit 0dcfc3e into develop Oct 12, 2023
36 checks passed
@mergify mergify bot deleted the wip/jd/excel-sheet branch October 12, 2023 14:30
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.

4 participants