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

Implement value formatting and writing new files in Delimited format. #3528

Merged
merged 38 commits into from
Jun 23, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Jun 14, 2022

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/182309429 and https://www.pivotaltracker.com/story/show/182309573

Important Notes

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 dist and ./run ide watch.

@radeusgd radeusgd self-assigned this Jun 14, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/delimited-write-new-182309573 branch 8 times, most recently from 6b8d386 to e59c554 Compare June 21, 2022 13:50
@radeusgd radeusgd force-pushed the wip/radeusgd/delimited-write-new-182309573 branch from ce11d77 to 1a30069 Compare June 23, 2022 07:09
@radeusgd radeusgd marked this pull request as ready for review June 23, 2022 07:09
Copy link
Member

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

Pushing first set of comments.

@radeusgd radeusgd requested a review from jdunkerley June 23, 2022 10:56
Copy link
Member

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

Minor points


@Override
public boolean canFormat(Object value) {
return value instanceof Double || value instanceof Long;
Copy link
Member

Choose a reason for hiding this comment

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

should this format Longs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question.

Currently I think this may not be strictly needed, because our Decimal-type column will contain all doubles and a mixed type column will delegate to the AnyObjectFormatter which will in turn delegate to IntegerFormatter for Longs. But theoretically if we had a mixed-storage column containing Longs and Doubles, and such a column had the DecimalFormatter selected - this functionality could be of use.

However, since currently it's not actively used it may be unnecessary to maintain it this way indeed. I can remove it.

@radeusgd radeusgd requested a review from jdunkerley June 23, 2022 11:23
@radeusgd radeusgd force-pushed the wip/radeusgd/delimited-write-new-182309573 branch from b1534f9 to 2d4537f Compare June 23, 2022 11:56
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jun 23, 2022
@mergify mergify bot merged commit 972b34d into develop Jun 23, 2022
@mergify mergify bot deleted the wip/radeusgd/delimited-write-new-182309573 branch June 23, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants