Skip to content

Use poetry for packaging & dependency management#31

Open
arnaudgelas wants to merge 1 commit into
councilofelders:masterfrom
arnaudgelas:dep
Open

Use poetry for packaging & dependency management#31
arnaudgelas wants to merge 1 commit into
councilofelders:masterfrom
arnaudgelas:dep

Conversation

@arnaudgelas

Copy link
Copy Markdown

No description provided.

@arnaudgelas arnaudgelas mentioned this pull request Nov 10, 2021
@arnaudgelas arnaudgelas force-pushed the dep branch 3 times, most recently from 1e4cacc to d732670 Compare November 10, 2021 19:27
Comment thread setup.sh
if ! command -v poetry &> /dev/null
then
echo "poetry is missing. Installing ..."
curl -sSL https://raw.githubusercontent.com/python-poetry/poetry/master/get-poetry.py | python -

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

executing some remote file is not exactly the safest way of installing things. Any other options of getting poetry, pip install poetry maybe? :-)

@uuazed uuazed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have never used poetry and I don*t fully get which problem it is solving, especially in case of opensignals. Like there wasn`t anything broken with the setup.py + requirements.txt way of doing things. But I like trying out new things, so why not... Ok from my end.

I*ve added a few comments and let @jrdi decide

Comment thread .pylintrc

# Specify a score threshold to be exceeded before program exits with error.
fail-under=10.0
fail-under=9.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are making pylint less strict?

Comment thread setup.sh
@@ -0,0 +1,19 @@
#! /usr/bin/bash

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume, we should all run this script to setup our dev environment. Could you add a short section to the README file to explain this?

- name: Publish distribution 📦 to PyPI
if: startsWith(github.ref, 'refs/tags')
uses: pypa/gh-action-pypi-publish@master
run: poetry publish

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we still need the setup.py and requirements.txt files, I guess they are deprecated with your PR, no?
And is the publish doing the same things, for example is the README used as the description on pypi?

@jrdi

jrdi commented Nov 13, 2021

Copy link
Copy Markdown
Collaborator

I have never used poetry and I don*t fully get which problem it is solving, especially in case of opensignals. Like there wasn`t anything broken with the setup.py + requirements.txt way of doing things. But I like trying out new things, so why not... Ok from my end.

I*ve added a few comments and let @jrdi decide

I tend to agree with @uuazed here, I don't fully understand the reason behind the PR and the problem is solving. Anyway, since the effort has been already put into the change, I'll be happy to merge once the comments are addressed.

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