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

Jump to bottom

[9660][twisted/internet/_glibbase] Fix Typeerror in simulate for py3 #1679

Merged
merged 3 commits into from Feb 8, 2022
Merged

[9660][twisted/internet/_glibbase] Fix Typeerror in simulate for py3 #1679

merged 3 commits into from Feb 8, 2022

Conversation

doadin
Copy link
Contributor

@doadin doadin commented Jan 3, 2022 •
edited

Scope and purpose

Fixes a Type error when calling simpulate() in a glib reactor.

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9660
  • 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 #1679 from doadin/patch-3

Author: Doadin
Reviewer:
Fixes: ticket: 9660

@doadin doadin force-pushed the patch-3 branch 2 times, most recently from ec06a1e to bf132b3 Compare January 3, 2022 06:37
@doadin doadin mentioned this pull request Jan 12, 2022
Closed
7 tasks
@doadin
Copy link
Contributor Author

doadin commented Jan 12, 2022 •
edited

@twm I know you reviewed this years ago when I first tried to submit this so, I guess I will ask you. What do I do when the required checks fail due to issues not related to my change?
@glyph

@doadin doadin force-pushed the patch-3 branch 12 times, most recently from b26d96c to 49cf9bd Compare January 12, 2022 06:51
@doadin
Copy link
Contributor Author

doadin commented Jan 12, 2022 •
edited

for failing test: python 3.6 is no longer included in macos image. And not within the scope of my changes.

@doadin doadin changed the title [#9660][twisted/internet/_glibbase] Fix Typeerror in simulate for py3 [9660][twisted/internet/_glibbase] Fix Typeerror in simulate for py3 Jan 12, 2022
cas-- pushed a commit to deluge-torrent/deluge that referenced this pull request Jan 13, 2022
@doadin @cas--
Build With Patched Twisted Build
490fb89 
Fixes TypeError in simulate call

Ref: https://twistedmatrix.com/trac/ticket/9660
Ref: twisted/twisted#1679
@doadin
Copy link
Contributor Author

doadin commented Jan 15, 2022

@glyph Nice all the checks pass now, is there anything I need to change?

@glyph
Copy link
Member

glyph commented Jan 15, 2022

Sadly, the project is suffering for a severe lack of reviewer bandwidth right now, so it's not your fault. If you have a look at https://twisted.reviews/ you might see some stuff submitted by project members that you can review, though, and that might relieve the load a little bit to get this looked at! You're under no obligation, of course, so you can just wait, but this is the reality of open source "staffing", for lack of a better word 😄 .

Personally I typically work from the top of that list down when I have time to look at a few things.

@doadin
Copy link
Contributor Author

doadin commented Jan 15, 2022

@glyph sorry for adding to the mass amount of messages I'm sure you get. Thanks!

adiroiban
adiroiban requested changes Feb 7, 2022
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Changes looks good. Thanks.

I left a few suggestions to improve the tests

The current testing code looks like it's not finished .
No docstring or empty docstring.

Please check my suggestions.

After that, I think that this is ready to review and merge.

Feel free to ping once ready.

Thanks again for your help!

src/twisted/internet/test/test_glibbase.py Outdated Show resolved Hide resolved
@doadin doadin force-pushed the patch-3 branch 3 times, most recently from d77dbab to b9ce4a3 Compare February 7, 2022 20:26
@doadin @adiroiban
[twisted/internet/_glibbase] Fix Typeerror in simulate for py3
7f907a8 
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@doadin doadin requested a review from adiroiban February 7, 2022 20:48
@doadin
Copy link
Contributor Author

doadin commented Feb 7, 2022

@adiroiban I rebased to merge some pre-commit/formating changes but I think its good now?

adiroiban
adiroiban approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Looks good. Many thanks!

glyph
glyph reviewed Feb 7, 2022
View reviewed changes
src/twisted/newsfragments/9660.bugfix Outdated
@@ -0,0 +1 @@
twisted.internet._glibbase.PortableGlibReactorBase.simulate no longer raises TypeError when there are no delayed called. This was a regression introduced with the migration to Python3 in which the builtin `min` function no longer accepts `None` as an argument.
Copy link
Member

Choose a reason for hiding this comment

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

The relevant public names are twisted.internet.gireactor.PortableGIReactor.simulate and twisted.internet.gtk2reactor.PortableGtkReactor.simulate; in general we should avoid news fragments that mention private names (those which have a single underscore directly following a dot)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so....should I change something or you guys gonna figure this one out?

Copy link
Member

Choose a reason for hiding this comment

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

We can go ahead and merge this as-is, my comment was more for @adiroiban than you :)

Copy link
Member

Choose a reason for hiding this comment

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

(Nothing wrong with fixing up the changelog later.)

@adiroiban
Copy link
Member

Is best to fix the release notes in the PR, when possible :)

@adiroiban adiroiban merged commit 9045ef7 into twisted:trunk Feb 8, 2022
@doadin
Copy link
Contributor Author

doadin commented Feb 8, 2022

@adiroiban @glyph
Thanks!

cas-- added a commit to cas--/Deluge that referenced this pull request Feb 9, 2022
@cas--
[CI] Replace custom twisted package with pre-release
50647ab 
Fix for Windows simulate error has been merged and in 22.2.0rc

Ref: twisted/twisted#1679
@yan12125 yan12125 mentioned this pull request Mar 18, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiroiban adiroiban adiroiban approved these changes

@glyph glyph glyph left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@doadin @glyph @adiroiban