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

Make pandas optional #43

Closed
wants to merge 1 commit into from

Conversation

gutzbenj
Copy link
Contributor

@gutzbenj gutzbenj commented Feb 2, 2024

Dear @gadomski ,

this PR makes pandas optional (still installable via isd[io])

@gutzbenj gutzbenj force-pushed the gutzbenj/pandas branch 3 times, most recently from 5019562 to 5f7f000 Compare February 2, 2024 17:00
@gutzbenj gutzbenj mentioned this pull request Feb 2, 2024
src/isd/batch.py Outdated Show resolved Hide resolved
src/isd/batch.py Outdated Show resolved Hide resolved
@gutzbenj gutzbenj force-pushed the gutzbenj/pandas branch 3 times, most recently from dafb6a3 to 3279e1b Compare February 9, 2024 15:58
@gutzbenj
Copy link
Contributor Author

Dear @gadomski ,

please re-review :)

Copy link
Owner

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

@gutzbenj I can't push to your branch, but can you take up the changes in b117e40? Rather than use PR suggestions, I just made some edits. Mostly removing the pytest mark b/c I don't think it's necessary, and some spurious whitespace changes as well. Thanks!

@gutzbenj
Copy link
Contributor Author

Dear @gadomski ,

I really can't seem to understand your policy here. You said before that

  • (regardless of the PR) you want to have as little diff as possible
  • if pandas is optional the lib should be tested with and without it being installed

though the commit you did both

  • introduces unnecessary diff by removing the blank lines
  • removes the steps in CI that test without and with pandas being installed

Also, the marker should just be helpful - which it is per se.

I'm really confused now, please clear my worries 😵‍💫

@gadomski
Copy link
Owner

I'm really confused now, please clear my worries 😵‍💫

Yeah, I apologize @gutzbenj, thanks for your patience. I only come back to this project every once in a while and sometimes I forget previous decisions I've made 🙃 . To avoid further confusion, I've rolled up both of these PRs with my current way of thinking into #47. Thanks for your contribution!

@gadomski gadomski closed this Mar 14, 2024
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.

None yet

2 participants