-
Notifications
You must be signed in to change notification settings - Fork 96
Implement Async #81
Implement Async #81
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.
Overall looks great. Some nitpicks here and there, but should be good to go after fixing that.
Though we need to do something about the formatting. @AhmedSoliman I'm not up-to-date with the status of the formatting. Can we format the repository with black (again?) to make sure our own contributing guidelines can be followed?
@AhmedSoliman hey can we drop Python 3.6 with this PR as well? This would make it so later's IsolatedAsyncioTestCase backport can be used. Python 3.6 is only getting security updates till December this year anyhow. Then we also don't need the check added in |
@AndreasBackx I don't mind dropping support for Python 3.6. Agreed. I'm not sure about the state of formatting but if this includes reformatting of existing code, that should be split into its own PR. Note that there is some black-specific configuration in https://github.com/facebookincubator/python-nubia/blob/main/pyproject.toml so there is a possibility that our internal "black" formatting isn't following those? |
beb65a4
to
b9d7b01
Compare
@AndreasBackx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
918fcdb
to
36ee7f2
Compare
@AndreasBackx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Following the comments on #81 this formats all the code using isort and black according to contributing guidelines. This is based on #82 Pull Request resolved: #84 Reviewed By: AhmedSoliman Differential Revision: D31893969 Pulled By: AndreasBackx fbshipit-source-id: faf270c04df851097ead934f0d5f01c363bd7fa2
This needs to be rebased after your formatting changes. |
36ee7f2
to
2fb1949
Compare
Done. Thank you for the opportunity to contribute! @AhmedSoliman |
@SelleslaghGianni glad you could pick it up ! |
@AndreasBackx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: I was looking for something to do when I came across this repo. I saw the stale PR and wanted to take a crack at it. As for the formatting done in some files, I use black and at first I didn't pay attention until I noticed it here. I read in the contributing file that black is being used, so I'm hoping those changes are OK. I tried to follow the comments given to the previous PR as closely as I could, but I may have done something completely wrong, so I am open for feedback. I've rebased the commits made by esciara and the other PR's I've made. Edit: This is based on facebookarchive/python-nubia#84 Pull Request resolved: facebookarchive/python-nubia#81 Reviewed By: AhmedSoliman Differential Revision: D31782837 Pulled By: AndreasBackx fbshipit-source-id: 0d64722f78b30712859bb8bdf1f4c6d5efb1f266
I was looking for something to do when I came across this repo. I saw the stale PR and wanted to take a crack at it.
As for the formatting done in some files, I use black and at first I didn't pay attention until I noticed it here. I read in the contributing file that black is being used, so I'm hoping those changes are OK.
I tried to follow the comments given to the previous PR as closely as I could, but I may have done something completely wrong, so I am open for feedback.
I've rebased the commits made by @esciara and the other PR's I've made.
Edit: This is based on #84