Skip to content

Add calamine Excel reader #898

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

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Add calamine Excel reader #898

merged 3 commits into from
Apr 9, 2024

Conversation

davetapley
Copy link
Contributor

Since pandas-dev/pandas#54998

No existing tests covering engine, I can add if needed?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Mar 31, 2024

Thanks for the PR. Would like opinion of @twoertwein to determine if we should add tests for engine keyword. On the one hand, it would be nice to have for completeness. On the other hand, we'd have to add a build dependency for calamine, which might be fine to do assuming all the installation issues work out for developers and in CI.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Mar 31, 2024

Also, CI is failing because a change was made in pyright 1.1.356, so you can lock down pyright to 1.1.354 in this PR in pyproject.toml. There's another PR out there that will allow us to upgrade pyright to 1.1.356

@twoertwein
Copy link
Member

we'd have to add a build dependency for calamine, which might be fine to do assuming all the installation issues work out for developers and in CI.

I'm not familiar with how python-calamine is packaged. Assuming its pypi package contains everything to test it, I don't see a reason not to make it a development dependency. If we have to install packages that are not on Pypi, I would be inclined to have tests inside a TYPE_CHECKING block instead.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Apr 1, 2024

we'd have to add a build dependency for calamine, which might be fine to do assuming all the installation issues work out for developers and in CI.

I'm not familiar with how python-calamine is packaged. Assuming its pypi package contains everything to test it, I don't see a reason not to make it a development dependency. If we have to install packages that are not on Pypi, I would be inclined to have tests inside a TYPE_CHECKING block instead.

It is on pypi and conda-forge, so I think we'd be OK. I was concerned that it is built on top of rust, and I don't know if that means a whole lot of other stuff gets installed as a result.

@davetapley Open to have you add tests to this PR

davetapley added a commit to JEFuller/pandas-stubs that referenced this pull request Apr 2, 2024
@davetapley davetapley requested a review from Dr-Irv April 2, 2024 21:42
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

Can you merge with upstream/main - CI is failing because of that. And update the pyright version as indicated.

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

thanks @davetapley

@Dr-Irv Dr-Irv merged commit 0d54b01 into pandas-dev:main Apr 9, 2024
13 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.

3 participants