Trying to slove #179 (aioredis 1.3.1 dropped explicit loop requirement in API)

[edelvalle]

Refs #179

[bjd183]

It looks like you need to fix the linter error by moving import sys into alphabetical order (after import string).

[edelvalle]

Nice! thanks!

[carltongibson]

OK, it's Sunday so... but... 🙂

I'm scratching my head just looking at this: we're checking the Python version before changing the call to aioredis. At first glance that doesn't seem right.

I'm sure if I read the linked issue through entirely it would make sense, but care to explain?
Thanks!

[bjd183]

aioredis implements similar logic here. I don't know if this is the right approach or not. The underlying loop parameter is deprecated in Python 3.8 where it now automatically determines the appropriate loop (I think). Simply dropping the loop parameter is likely to break Python 3.6-3.7.

[bjd183]

Would it be preferred to check the version of aioredis?

…

On Sun, Mar 1, 2020 at 8:37 AM Carlton Gibson ***@***.***> wrote: ***@***.**** commented on this pull request. OK, it's Sunday so... but... 🙂 I'm scratching my head just looking at this: we're checking the Python version before changing the call to aioredis. At first glance that doesn't seem right. I'm sure if I read the linked issue through entirely it would make sense, but care to explain? Thanks! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#180?email_source=notifications&email_token=ADY6MCNLZUZRJDZT6S2YSYLRFKFMVA5CNFSM4KSEZBQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXPAD3I#pullrequestreview-366871021>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADY6MCLJ35YLC7U44ILTNNDRFKFMVANCNFSM4KSEZBQQ> .

[edelvalle]

I think you are right, is a combination of the version of python and aioredis. I have to check both.

right?

[bjd183]

@edelvalle Perhaps if sys.version_info >= (3, 8, 0) and tuple(map(int, aioredis.__version__.split('.'))) >= (1, 3, 1): will be acceptable.

[edelvalle]

@bjd183 thanks for the advice, I was busy but finally managed to do as instructed by you.
btw, also made black tell what's the problem when it gets angry with our code 😄

[bjd183]

@carltongibson how does this look?

[carltongibson]

Hi both, thanks for the work here. Looks good.

Currently in a tight spot. 🦠 disrupting pretty much everything, so I've had ≈0 time to look at anything. I have a commitment to DRF on my list and then I'll have the capacity for Channels &co.

If you have a bit of time to look over the other issues/PRs (maybe with @tarikki, who's been active) then I'm happy to take group consensus on issues and we can put out a release as first job back. (Equally, no stress if you don't have such capacity.)

[carltongibson]

OK, this looks sensible.

Can you split it into 2 commits? One for the CI change, and one for the fix.

Good work! 👍

[bjd183]

What is the best way to reduce to two commits? Squash usually results in a single commit, right? Can this be done with rebase? Or does it require cherry-pick? And are any of these possible now that the changes have already been committed and pushed?

[bjd183]

Also, (I've not done this before) is it possible for me to contribute to a pull request that I do not own? How would I do so? It is not obvious to me via this interface.

[bjd183]

@edelvalle you could also revert the CI change to black and limit this PR to the fix so it can be easily squashed.

[wmorrell]

@bjd183 How I would split one commit into two:

1. Checkout branch containing the commit to split: git checkout edelvalle/master
2. Make new branch from the original: git checkout -b gh-179
3. Rewind the commit, so HEAD is at the parent and the changes in the commit are in working copy: git reset HEAD~
4. Add in changes for first commit with: git add and/or git add -p
5. Do the first commit: git commit
6. Add in changes for second commit: git add
7. Do the second commit: git commit
8. Push
9. Beverage

[carltongibson]

Thanks @wmorrell. (Been knocked sideways by the current situation so had little time...)

[bjd183]

Alright, I gave that a try. I had to fork edelvalle/master since I cannot commit directly to it. I was able to rewind and reset to squash commits to just two. But I wasn't able to push without an additional merge commit since I was then behind the remote tracking branch. I created a PR on edelvalle/master which, once merged, I assume propagates to this PR. I'm curious to see how this works since the original set of multiple commits still exists in the history.

[edelvalle]

This is worse, now I have even more commits, let me try to undo all this

[wmorrell]

Oops, I should have mentioned that it's usually cleaner to re-submit a pull request than to alter (someone else's) existing request. I tried to go for that by adding instruction to make a new branch, but failed to take the suggestion all the way to new PR. I also should have suggested squashing before the git reset HEAD~, as it doesn't work very well to reset last commit when the upstream has a small handful of commits instead of one :)

@edelvalle, I will do a fork a bit later to get just the two commits requested. If you would like to give it a shot, I suggest use of a few rounds of git rebase -i origin/master along with the git reset HEAD~ and git add -p tools. I'll keep a log of the commands I use, so hopefully it helps someone in the future.

[bjd183]

@wmorrell Thanks for the clarification.

[carltongibson]

Closing in favour of #193. Thanks all!