Skip to content

Conversation

Sikerdebaard
Copy link

Hi all!

Here's a pr for adding the get_product api call.

It also includes a requirements.txt and setup.py that are more of an example than something that should be merged blindly. Poetry is nice, but it is not very pythonic and it integrates poorly with data science workflows, e.g. jupyter notebooks, as poetry does not support pip install -e-type functionality.

@top-on
Copy link

top-on commented Jun 13, 2021

hi @Sikerdebaard, as an interested follower of this project, it is not my call to decide on whether to use pyproject.toml or setup.py in this project.
i would just like to point out two cool features about poetry that you might be overlooking, but which I have come to enjoy:

  • poetry can work with jupyter. this is especially nice when jupyter is wrapped into an IDE, see here.
  • when installing a local dependency with develop = true (e.g. my-package = {path = "../my/path", develop = true}), see here, this is pretty similar to pip install -e. in the case for poetry, you might have to run poetry install for it to refresh the local dependency.

@MikeBrink
Copy link
Owner

Hi thanks for your contribution!

First, I’ll have a look at your get_product addition, as it seems to fail some tests.

Second, I’d like to keep using poetry to manage the dependencies, I think it is easier from a maintainers perspective. I get that it might not give hassle for users, but I’d like to point two things out regarding this.

  1. Most users can install this package using pip and its dependencies using pip install python-picnic-api
  2. Users whom both want to use the most up-to-date version + want to use requirements.txt are able to do so using poetry export -f requirements.txt --output requirements.txt As seen here.

I hope that I have explained my views on the matter sufficiently! :)

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