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

Small changes from Book Club issues #6533

Merged
merged 17 commits into from
May 6, 2023
Merged

Small changes from Book Club issues #6533

merged 17 commits into from
May 6, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented May 3, 2023

Pull Request Description

  • Add dropdown to tokenize and split column.
  • Remove the custom Join_Kind dropdown.
  • Adjust split and tokenize names to start numbering from 1, not 0.
  • Add JS_Object serialization for Period.
  • Add days_until and until to Date.
  • Add Date_Period.Day and create next and previous on Date.
  • Use simple names with File_Format dropdown.
  • Avoid using Main.enso based imports in Standard.Base.Data.Map and Standard.Base.Data.Text.Helpers.
  • Remove an incorrect import from Standard.Database.Data.Table.

From #6587:

A few small changes, lots of lines because this affected lots of tests:

  • Table.join now defaults to Join_Kind.Left_Outer, to avoid losing rows in the left table unexpectedly. If the user really wants to have an Inner join, they can switch to it.
  • Table.join now defaults to joining columns by name not by index - it looks in the right table for a column with the same name as the first column in left table.
  • Missing Input Column errors now specify which table they refer to in the join.
  • The unique name suffix in column renaming / default column names when loading from file is now a space instead of underscore.

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 changed the title - Add dropdown to tokenize and split column. Small changes from Book Club issues May 3, 2023
@jdunkerley jdunkerley force-pushed the wip/jd/table-dry-run branch 2 times, most recently from a638ad6 to 6526b2d Compare May 5, 2023 08:52
@jdunkerley jdunkerley marked this pull request as ready for review May 5, 2023 09:10
@jdunkerley jdunkerley added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels May 5, 2023
Comment on lines +9 to +21
public static TemporalAdjuster day_start =
(Temporal temporal) -> {
return temporal.isSupported(ChronoField.NANO_OF_DAY)
? temporal.with(ChronoField.NANO_OF_DAY, 0)
: temporal;
};

public static TemporalAdjuster day_end =
(Temporal temporal) -> {
return temporal.isSupported(ChronoField.NANO_OF_DAY)
? temporal.with(ChronoField.NANO_OF_DAY, NANOSECONDS_IN_DAY - 1)
: temporal;
};
Copy link
Member

Choose a reason for hiding this comment

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

This logic seems concerning to me.

I guess it should be fine, but what if we have a date object that supports hour/minute/second but not nanosecond? It will not be adjusted by this.

Shouldn't we just throw if temporal.isSupported returns False?

I assume you maybe want this to work like identity on Date - OK. But maybe then we can do some more robust check for if this supports <1day granularity?

Maybe it's ok, but it just feels to me that there are more possibilities than "support nanos of day" vs "not support day parts at all" and so it seems safer to handle the 'possibility in the middle' at least by throwing an exception that we did not expect it, instead of returning identity. But maybe I'm a bit too picky here.

@@ -229,6 +229,22 @@ spec_with name create_new_date parse_date =
(create_new_date 2000 7 1).end_of Date_Period.Quarter . should_equal (Date.new 2000 9 30)
(create_new_date 2000 6 30).end_of Date_Period.Quarter . should_equal (Date.new 2000 6 30)

Test.specify "should allow to compute the number of days until a date" <|
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add tests for the next method?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I want to add tests for next, previous and until.

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, as long as the widgets work in the IDE :)

I guess ideally would be good to attach some screenshots if possible.

@jdunkerley jdunkerley force-pushed the wip/jd/table-dry-run branch from b4cb90e to 2022874 Compare May 5, 2023 12:05
- end: the end date of the interval to count workdays in.
- include_end_date: whether to include the end date in the count.
By default the end date is not included in the interval.
days_until : Date -> Boolean -> Integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this logic could be part of Period so it could be used in more ways.

@jdunkerley jdunkerley added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels May 5, 2023
@jdunkerley jdunkerley added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 5, 2023
…to ` n` (#6587)

* change defaults

* update tests

* update tests

* change _ to ' '

* WIP - add location

* finish problem builder refactor

* adapting other places and tests

* docs
@jdunkerley jdunkerley removed the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 6, 2023
@mergify mergify bot merged commit bc0db18 into develop May 6, 2023
@mergify mergify bot deleted the wip/jd/table-dry-run branch May 6, 2023 10:10
Procrat added a commit that referenced this pull request May 10, 2023
* develop: (28 commits)
  Add tests for Date.until, Date.next and Date.previous. (#6606)
  Improve `Non_Unique_Primary_Key` error, split file format detection into read/write, improve SQLite format detection (#6604)
  tokenize_to_columns or parse_to_columns results in a single column we shouldn't add the  1 (#6607)
  Fix node editing race condition (#6594)
  Add format to the in-memory Column (#6538)
  Fix dashboard issues (part 2) (#6511)
  Fix visualisation type selector artifacts rendered after node preview visualisation was closed. (#6575)
  Revert typescript CI Lint changes (#6602)
  Fix the Engine version check in GUI (#6570)
  Show error pop-up when failing to rename a project (#6366)
  Small changes from Book Club issues (#6533)
  "at_least_one" flag for tokenize_to_rows (#6539)
  Benchmark Engine job runs only engine, not Enso benchmarks (#6534)
  Catch 5813 and avoid crash (#6585)
  Fix opening links in desktop IDE (#6507)
  Identify SyntaxError exception and avoid printing a stack trace (#6574)
  Fix dashboard issues (#6502)
  Let ChangesetBuilder.invalidated search even container elements (#6548)
  Fix #5075: stop panning on full-screen visualisation (#6530)
  Only `Join_Kind.Inner` removes the common-named columns (#6564)
  ...
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.

3 participants