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

Add read of Arrow TimeStamp without timezone as LocalDatetime #515 #516

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

fb64
Copy link
Contributor

@fb64 fb64 commented Nov 24, 2023

Implementation of Arrow TimeStamp types (without time zone)

@Jolanrensen Jolanrensen self-requested a review November 27, 2023 12:12
@Jolanrensen Jolanrensen added the enhancement New feature or request label Nov 27, 2023
Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

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

Thanks for the addition and the tests!
Please lint your (test) files, otherwise our CI will fail. You can test this by running gradlew lintKotlin.
In the future we should probably think about using Kotlinx LocalDateTime instead of the Java one, but since the Java variants are already used in the other Arrow functions, this is probably fine for now.

@Jolanrensen
Copy link
Collaborator

After #521 is merged, please rebase, fix the linting issues and then it can be merged :)

@Jolanrensen
Copy link
Collaborator

It's merged, please rebase so we can merge your branch too :)

@fb64 fb64 force-pushed the master branch 2 times, most recently from d4ddc73 to 9d3e4e1 Compare November 30, 2023 13:47
@fb64
Copy link
Contributor Author

fb64 commented Nov 30, 2023

It's merged, please rebase so we can merge your branch too :)

I think it's ok, you can check and merge

Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

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

Almost :) sorry to nitpick.
The CI tests I ran successfully on https://github.com/Kotlin/dataframe/tree/fb64-master btw :)

@fb64
Copy link
Contributor Author

fb64 commented Nov 30, 2023

Almost :) sorry to nitpick. The CI tests I ran successfully on https://github.com/Kotlin/dataframe/tree/fb64-master btw :)

no problem, it's weird I ran formatKotlin and full build was ok. I don't know this parser maybe it's a missing check 🤔
It should be the ok by now 🤞

@Jolanrensen
Copy link
Collaborator

Almost :) sorry to nitpick. The CI tests I ran successfully on https://github.com/Kotlin/dataframe/tree/fb64-master btw :)

no problem, it's weird I ran formatKotlin and full build was ok. I don't know this parser maybe it's a missing check 🤔 It should be the ok by now 🤞

Thanks! I suspect it's because we had to turn off a linter rule because it broke in some scenario. For this specific one I think it's because we allow notations like df[{ something }] which the linter tries to fix to df[ { something}]...
One day we'll switch to a better solution hopefully: #364

@Jolanrensen Jolanrensen self-requested a review December 1, 2023 11:56
@Jolanrensen Jolanrensen merged commit 63fdb46 into Kotlin:master Dec 1, 2023
@fb64
Copy link
Contributor Author

fb64 commented Dec 1, 2023

Well, you can close the related issue #515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants