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 saving/loading spreadsheets to/from xlsx #417

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

rokostik
Copy link
Contributor

@rokostik rokostik commented Nov 24, 2024

This PR implements spreadsheet <-> xlsx using the excelize library.

When saving the types are correctly saved in the xlsx.

When loading the fist sheet is used to load (should we parametrize this?). The loaded spreadsheet values are all strings. The library has the option to get a cell type but it's a bit cumbersome to use (it needs to be done for each cell in the excel cell index, eg. "A1". I can of course change it but I'm wondering if combining the current implementation with |autotype is good enough?

I've also added a mktmp function to the os context for easier testing.

@refaktor refaktor merged commit 5b9b0f5 into refaktor:main Nov 24, 2024
7 checks passed
@refaktor
Copy link
Owner

Great! I think if the spreadsheet has type information, an expected behavior would be that the types are transferred to the Rye spreadsheets and vice versa. If everything is just string loose information from xlsx and then we try to guess it again based on the format, which can cause rare but because of it even more unexpected bahaviour.

In Rye spreadsheet each cell is also typed separately (there is no per-column type information) so converting spreadsheets cell values to rye values of appropriate type makes sense to me.

@rokostik
Copy link
Contributor Author

so converting spreadsheets cell values to rye values of appropriate type makes sense to me.

Noted, I'll prepare another PR for that change.

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.

2 participants