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

Expand and improve pretty for core data types, vector and table. #11438

Merged
merged 27 commits into from
Oct 31, 2024

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Oct 29, 2024

Pull Request Description

  • ✅ Alter default Any.pretty so constructor is prefixed with type name (as needed now).
    image
  • ✅ Tests for pretty on Date.
  • pretty for ✅ Date_Time and ✅ Time_Of_Day improved to not have as much noise.
  • pretty for ✅ Period, ✅ Date_Range and ✅ Range.
  • Added custom pretty for ✅ Vector and ✅ Array as built-in method doesn't call through to overrides.
  • Added custom pretty for ✅ Column and ✅ Table.
  • Bug fix for pretty in Time_Zone so calls through to pretty of the zone_id to ensure safely escaped.
  • Initial default_widget for Date and Time_Of_Day.
  • Improve widget for Date.to_date_time.
    image
  • to_text, to_display_text and pretty for Enso_Secret
    image
  • private constructor for Enso_Secret as can't be correctly built directly.
  • Use _ for the testing methods in HTTP to clarify they shouldn't be used in general code.

Important Notes

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,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@jdunkerley jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 29, 2024
Date_Range.pretty fix.
Fix `Date_Range.pretty`.
Tests for pretty on `Period` and `Date_Range`,
Tests for `Date_Time.pretty`.
Test for `Vector.pretty`.
group_builder.specify "should convert to Enso code" <|
create_new_datetime 1970 . pretty . should_equal "Date_Time.new 1970 1 1"
create_new_datetime 1923 9 24 . pretty . should_equal "Date_Time.new 1923 9 24"
create_new_datetime 1923 9 24 12 20 44 . pretty . should_equal "Date_Time.new 1923 9 24 12 20 44"
Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate a test where hours and minutes are 0, so that we get the seconds=

from Standard.Base.Network.HTTP import _resolve_headers, get_follow_redirects, get_proxy, get_timeout, get_version
from Standard.Base.Network.HTTP import _resolve_headers, _get_follow_redirects, _get_proxy, _get_timeout, _get_version
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the convention for _ was to indicate module private methods?

Not opposed to using _ for 'internal method that is exported for test purposes', just making sure we have a clear guideline on the naming.

Also, I guess if we want to do this proper, we could mark these methods as private and move these few test cases into Base_Internal_Tests that can load private modules.

But ofc happy to proceed as-is for now.

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'm hoping we will get tests inside the same project soon and then these would be properly private.

Debug.eval c1.pretty . should_equal c1

c2 = Column.from_vector "X" ["a", 42]
c2.pretty . should_equal 'Column.from_vector \'X\' [\'a\', 42]'
Copy link
Member

Choose a reason for hiding this comment

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

very minor nitpick, but wouldn't this test be more easily readable with "?

Suggested change
c2.pretty . should_equal 'Column.from_vector \'X\' [\'a\', 42]'
c2.pretty . should_equal "Column.from_vector 'X' ['a', 42]"

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 great overall, just some minor suggestions

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Oct 31, 2024
@mergify mergify bot merged commit 610ee5f into develop Oct 31, 2024
42 checks passed
@mergify mergify bot deleted the wip/jd/column-table-pretty branch October 31, 2024 10:04
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