-
Notifications
You must be signed in to change notification settings - Fork 277
Update Makefile for python client with auto setup #1995
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
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.
@MonkeyCanCode Thanks for improving the Makefile! Overall LGTM
There are some discussion around doing the code check/format in a different way: #1986. Since the current one is an improvement of the existing solution, I suggest we seek to merge this first and change later based on the result of #1986
Exactly, it is a lot harder to do without this piece first (thinking about using native shell and markfile route instead of a third party plugin which may not be supported after 1-2 years). |
@HonahX please take another look when you get a chance. i added a helper for Makefile as well (update the PR summary to reflect the same):
|
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.
LGTM! Just have one comment about the poetry version in installation script
Thanks for improving this!
VERSION ?= $(shell cat pyproject.toml | grep version | sed 's/version *= *"\(.*\)"/\1/') | ||
BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%S%:z") | ||
GIT_COMMIT := $(shell git rev-parse HEAD) | ||
POETRY_VERSION := $(shell cat pyproject.toml | grep requires-poetry | sed 's/requires-poetry *= *"\(.*\)"/\1/') |
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 poetry version in pyproject.toml seems not having an upper bound
requires-poetry = ">=2.1"
I think in the installation script we might want to pin the exact version to avoid CI failures due to breaking changes in automatic version update.
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.
So I did thought about this, there are couple reasons on why I did the above:
There are 3 places where we are specify poetry (README.md within this direction...which we said >2.0 will work), regretest (which we defined a specific version, in this case 2.1.3), and pyproject.toml where we said anything >= 2.1 should work. I had updated all 3 pieces to be the same version. Please take another look when you get a chance.
The current
Makefile
andREADME.md
is asking users to setup needed python environment (either virtual or the one installed on the OS) then installed the needed dependencies. I think it makes more senses to automate it and use a virtual env instead to avoid change an end-users' OS python (as if they didn't setup virtual env, the package will needed to be installed on the OS one).This PR does the following:
Makefile
with venv setupcli/*
as well. The current one only ran againstintegration_tests/*
mypy
doesn't appear to be happy withcli/*
. Add a files regex for now to skip this check (so existed on withintegration_tests/*
will still get runmake lint
(2 files modified)Here is the sample output:
Here is the sample helper from Makefile: