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

Fix for #573: Change serialization format for rendering in IntelliJ IDEA #574

Merged
merged 12 commits into from
Mar 8, 2024

Conversation

ermolenkodev
Copy link
Contributor

@ermolenkodev ermolenkodev commented Jan 26, 2024

Fixes #573

@koperagen
Copy link
Collaborator

Can i ask you to move internal and private functions from this file to new writeJson.kt / readJson.kt files under impl.io package? This file became too big to navigate :c

@koperagen
Copy link
Collaborator

You changed handling of value and array columns. Can you tell me more about changes? This part of serialization was not tested by unit tests thoroughly before. I worry if things might break
(no test coverage)
image

@Jolanrensen Jolanrensen changed the title Fix for #573 Fix for #573: Change serialization format for rendering in IntelliJ IDEA Jan 29, 2024
…lugin

* Added the necessary metadata to the serialization format of the dataframe for it to be rendered in the Kotlin notebooks plugin.
* Introduced the method `toJsonWithMetadata` which includes the metadata during the serialization process of the dataframe.
This commit includes a new document that explains the JSON serialization format used for rendering dataframes in the Kotlin notebooks plugin for IntelliJ IDEA. The format does not adhere to any formal schema such as Json Schema, but is intended for illustrative purposes.
This update introduces specific rendering flows based on the version of the IDE. A new method has been added to retrieve the IDE build number. This information is then used to conditionally generate a JSON-encoded DataFrame, adapting to the specific capabilities of the IDE version in use. Additionally, relevant tests have been updated to accommodate these changes.
Changed the condition check in the fromString function. Now, it accepts build numbers with three or more parts.
* Renamed encodePrimitiveData to encodeValue
* Changed return type of toJsonWithMetadata to not expose JsonObject in publec API
* Moved internal and private functions from json.kt to new writeJson.kt / readJson.kt files under impl.io package
@ermolenkodev
Copy link
Contributor Author

Can i ask you to move internal and private functions from this file to new writeJson.kt / readJson.kt files under impl.io package? This file became too big to navigate :c

done

@ermolenkodev
Copy link
Contributor Author

You changed handling of value and array columns. Can you tell me more about changes? This part of serialization was not tested by unit tests thoroughly before. I worry if things might break (no test coverage) image

I have extracted the code that filters autogenerated values and array columns into functions to be reused in new methods. I have double-checked the logic, and it seems it hasn't been altered. However, I can't guarantee this without running tests. If this is critical, we should add tests and merge them before proceeding with this pull request.

Copy link
Collaborator

@koperagen koperagen left a comment

Choose a reason for hiding this comment

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

Approved, just one tiny fix related to take before merge please

Replaced the usage of 'rows().take().toDataFrame()' with the more concise 'take()' method in multiple files.
@ermolenkodev ermolenkodev merged commit 9e12d86 into Kotlin:master Mar 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change serialization format for rendering in IntelliJ IDEA
4 participants