-
Notifications
You must be signed in to change notification settings - Fork 2
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
New version #60
New version #60
Conversation
Note that appveyor is replaced by Github Actions, for which all tests pass: |
@xuhdev Kindly requesting for a review |
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.
Thank you for your contribution! It's actually pretty hard to review because so many different changes are mixed together in one giant PR. There are some changes that I think should be discussed separately (e.g., deleting renovate-lint.yml
). However, most of the PR should be looking good. Would you mind breaking down the PR a bit so we can get them reviewed separately?
Thank you again!
Good point, I'll split up the changes |
a561856
to
ae30c7b
Compare
@xuhdev The commits are now rearranged into meaningful and more atomic changes. Moreover, I've reverted the removal of renovate and editorconfig so that those changes can be discussed separately. |
f0e7087
to
1efe4ba
Compare
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.
Thanks again for the PR! I'll try to review one commit per day due to my lack of time.
Actually, you can create stacked PRs -- one PR starts on another PR's branch, and so on. In this way, there won't be conflict. I can give you permission to create branch and change non-master branches in this repo so things should be easier.
@@ -1,59 +0,0 @@ | |||
# Copyright (c) 2020 Hong Xu <[email protected]> |
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 reason that I had this tox.ini here is to avoid overdepending on a particular CI system. The appveyor file is pretty minimal and the heavy work is done in this tox.ini file.
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.
Ok, in that case let us stick with tox
and make it work with Github Actions. The benefit of this would be a shorter feedback loop, while maintaining portability.
Will split up PR |
I'm OK with using pre-commit, but it looks like pre-commit is not good at managing multiple python versions. I saw some people suggest using pre-commit in tox to get the best of both worlds, e.g., https://stackoverflow.com/a/71023703/1150462.
What do you think?
Hong
…On January 19, 2023 12:43:48 AM PST, Simon Brugman ***@***.***> wrote:
@sbrugman commented on this pull request.
> @@ -1,59 +0,0 @@
-# Copyright (c) 2020 Hong Xu ***@***.***>
Understandable. With all linting wrapped in pre-commit the Github Actions workflow is also relatively simple and could be easily ported to another CI. We could still wrap them in tox if you prefer.
|
I agree. In the other PRs I have integrated pre-commit with tox |
Major update with new feature and dealing with technical debt. Would be awesome if you're open to merging and releasing this!
pyproject.toml
pre-commit
for linting