Skip to content
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

[10187] Fix IRC ctcpExtract for pypy 3.7.4. #1592

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Conversation

adiroiban
Copy link
Member

@adiroiban adiroiban commented Apr 28, 2021

Scope and purpose

Our CI is failing in trunk after the auto-upgrade to pypy 3.7.4 which uses py3.10.

The IRC tests are failing.

I have found that the source is twisted.words.protocols.irc.ctcpExtract

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: adiroiban
Reviewer: wiml
Fixes: ticket:10187

Update  twisted.words.protocols.irc.ctcpExtract for PYPY 3.7.4 new list and iterator behaviour.

@@ -3678,10 +3678,10 @@ def ctcpExtract(message):
normal_messages.append(messages.pop(0))
odd = not odd

extended_messages[:] = filter(None, extended_messages)
normal_messages[:] = filter(None, normal_messages)
extended_messages[:] = list(filter(None, extended_messages))
Copy link
Member Author

Choose a reason for hiding this comment

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

My suggestion is to have something like this

Suggested change
extended_messages[:] = list(filter(None, extended_messages))
extended_messages = list(filter(None, extended_messages))

I don't know why we need to update the list "in-place"

Copy link
Contributor

Choose a reason for hiding this comment

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

@adiroiban because the two lists are inserted into the retval dictionary earlier. (I'm not sure if it's written that way for a reason... The whole function could probably be tidied up a lot using some list comprehensions, if someone wanted to do that.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah... I see.

so the issue is here

extended_messages = []
normal_messages = []
retval = {"extended": extended_messages, "normal": normal_messages}

and the return dict could have been created at the end with

return {"extended": extended_messages, "normal": normal_messages}

I think that we can live with that.

@adiroiban adiroiban requested a review from a team April 28, 2021 10:25
Copy link
Contributor

@wiml wiml left a comment

Choose a reason for hiding this comment

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

LGTM. The style of ctcpExtract is a bit odd I agree but I don't think it's important to clean it up beyond what's here.

@glyph
Copy link
Member

glyph commented Apr 29, 2021

I will land this since it's blocking builds.

@glyph glyph merged commit dc303ab into trunk Apr 29, 2021
@glyph glyph deleted the 10187-ctcpExtract-list-fix branch April 29, 2021 06:05
@adiroiban
Copy link
Member Author

For reference upstream bug report https://foss.heptapod.net/pypy/pypy/-/issues/3451

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.

None yet

3 participants