-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use Poetry for dependency management #148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few comments inline, and the following:
pytest
, not py.test
, has long been the recommended entry point for Pytest. This occurs in several places in this codebase. Suggest updating throughout.
README.md contains some outdated stuff, including
- mention of TravisCI
- directions in Releasing section involving setup.py; needs updating to pyproject.toml
- uses: actions/checkout@v2 | ||
- uses: actions/setup-python@v2 | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-python@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existence of v4 is worth mentioning to all of us in Zulip or at the next team meeting. Are there any noticeable differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't research any differences... just saw a deprecation notice w.r.t. v2 so I upgraded to the most recent version. It's probably worth mentioning to folks, even if I'm not very well informed about it aside from "it works!".
- uses: actions/checkout@v2 | ||
- uses: actions/setup-python@v2 | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-python@v4 | ||
- uses: psf/black@stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found it better to install Black (with a specific version) as a dev dependency on and invoke it using Python in the workflow script and from the command line or IDE when developing. Otherwise different versions of Black disagree on their defaults (@stable
does change), and it is very frustrating. Further, different repos following this patter use different versions, so it is not possible at present to have one global version on your machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point to a similar config in another repo that we could adopt in this one? (I'm not finding a lint.yml in any other repositories).
My approach to using black
has essentially been asking it to fix our formatting for us, and its version hasn't been of any consequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible that Black has settled down lately, but in the past it changed its default settings from release to release. Which caused angst when it changed under you -- potentially hundreds of changes in a formerly Black-compliant codebase. Hence version pinning. And making it part of the dev deps, so that everyone working on the repo, plus the workflow, is using the same Black, always.
I've noticed that Black checks are often the last step of the Python CI workflow. For example, in pycds. I believe this is because a failing Black check can cause your unit tests to be cancelled, and this can be undesirable. I think I recall discussing this with Nik a while back when we were both working on the same repo. In any case, it is common practice in our projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue that it's a good thing for black checks to be done first since they are the cheapest and that we consider them to be important for controlling code quality.
If there is a specific version of black that we should pin to, please feel free to recommend it :)
Going to merge this now, but will file an issue w.r.t. the reviewer's comments about specifying a version of |
Changes
This PR removes the use of Pipenv for dependency management and replaces it with Python poetry
setup.py
has been migrated topyproject.toml
pytest.cfg
has been incorporated intopyproject.toml
pyproject.toml
(which requires a module and function as an entry point rather than a script).