-
Notifications
You must be signed in to change notification settings - Fork 189
Add pre-commit as a Flake8 and MyPy delegate #91
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
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
@ebyhr, @joshthoward, hey, can I ask you to check the PR and the CLA? I’ve sent the CLA email a week ago but the check is still failing so I assume it wasn’t reviewed. |
@martint Could you make sure CLA email from @arturdryomov ? |
@ebyhr, the CLA was accepted this night. I’ll have to force-push to trigger the check I guess. |
@cla-bot check |
Your account is member of Trino org, but I can't find your account in contributor list.
We can trigger the check by above comment. |
@ebyhr, thanks! Yeah, seems like I was invited into the org but wasn’t added to |
@arturdryomov You don't need to open a PR. @martint will add your account to the list. |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
Rebased and the CLA seem to be in order. @ebyhr, @findepi, PTAL. Please tag related people, I’ve chosen you based on recent activity. |
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.
Minor change requested. Otherwise, looks good.
@@ -22,16 +22,19 @@ | |||
|
|||
|
|||
with open("trino/__init__.py", "rb") as f: | |||
version = str( |
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.
This seems like unnecessary error handling. We if the version is missing I would want an explicit failure.
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.
Switched to assert
, should fail and highlight the issue if it appears.
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.
This looks good to go % the 2 comments.
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 but the discussion around pre-commit is not resolved yet, right (cc: @hashhar)? I'm fine with the current approach though.
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 % workflow step name.
Sorry for the long back and forth @arturdryomov . Thanks for working on this.
.github/workflows/ci.yml
Outdated
@@ -3,6 +3,21 @@ name: ci | |||
on: [push, pull_request] | |||
|
|||
jobs: | |||
pre-commit: |
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.
@arturdryomov Can you give this a name like lint
? pre-commit is misleading.
Thank you @arturdryomov . Can you please squash the commits - I'll merge once the checks complete. |
@hashhar, done. Thanks for your help! |
Relates to #69. Also see
pre-commit
.