Skip to content

Replace asyncio.coroutine attributes with async def #298

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

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Conversation

srh
Copy link
Contributor

@srh srh commented Oct 4, 2023

This cherry-picks a change from #284 (thanks @stephanelsmith) and then removes commented lines.

This PR depends on #296, because it would conflict with its loop parameter removal changes (which is also redundant with changes in #284).

(Why not just merge #284 instead of these? Well, it is bigger than necessary. It commits ql2_pb2.py, it has some signal.signal changes I haven't looked at and understood yet, and it makes some changes around collections.abc that I think are a good cleanup, but I simply don't want to touch if not necessary.)

Checklist

@codeclimate
Copy link

codeclimate bot commented Oct 4, 2023

Code Climate has analyzed commit 405cd5d and detected 0 issues on this pull request.

View more on Code Climate.

@stephanelsmith
Copy link

Important update. Much appreciated you moving this forward.

This was referenced Oct 5, 2023
Copy link

@stephanelsmith stephanelsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change from yield from to await is long overdue. thumbs up

@stephanelsmith
Copy link

I don't remeber why I changed collections.abc.Callable in my PR. It clearly wasn't work for my application, I agree with the one step at a time approach.

@srh
Copy link
Contributor Author

srh commented Oct 6, 2023

I don't remeber why I changed collections.abc.Callable in my PR. It clearly wasn't work for my application, I agree with the one step at a time approach.

Looking again, I don't like the approach of assigning over collections.abc.Callable in some other library either -- what I liked about it was that it handled just the three classes that we used without renaming a module to another common module name. I'd probably do something like this for the three classes that we use:

try:
    Callable = collections.abc.Callable
except AttributeError:
    Callable = collections.Callable

Actually, this would also make me happy, both importing the module as "colls" -- the problem is the confusion of using an existing module name.

if sys.version_info < (3, 3):
    # python < 3.3 uses collections
    import collections as colls
else:
    # but collections is deprecated from python >= 3.3
    import collections.abc as colls

I don't think we should touch it at all though, because (a) there isn't active maintenance that, through higher productivity, justifies minor cleanups like this, and (b) there is another branch that probably would get worse merge conflicts.

lsabi
lsabi previously approved these changes Oct 9, 2023
Copy link
Contributor

@lsabi lsabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@srh srh dismissed lsabi’s stale review October 9, 2023 20:11

The merge-base changed after approval.

lsabi
lsabi previously approved these changes Oct 9, 2023
@lsabi lsabi self-assigned this Oct 9, 2023
@srh srh dismissed lsabi’s stale review October 9, 2023 20:59

The merge-base changed after approval.

@srh
Copy link
Contributor Author

srh commented Oct 9, 2023

I didn't do it. I guess Github does that automatically.

image

@lsabi lsabi self-requested a review October 9, 2023 21:05
lsabi
lsabi previously approved these changes Oct 9, 2023
@srh srh dismissed lsabi’s stale review October 9, 2023 21:06

The merge-base changed after approval.

@lsabi
Copy link
Contributor

lsabi commented Oct 9, 2023

@srh no worries

I don't understand why it's not working... @stephanelsmith can you review it and if ok approve it? Maybe it requires your review..

@srh
Copy link
Contributor Author

srh commented Oct 9, 2023

I've just rebased and force-pushed; I'll override the review and manually merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants