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

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

Merged
merged 3 commits into from Feb 8, 2022

Conversation

doadin
Copy link
Contributor

@doadin doadin commented Jan 3, 2022

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
Copy link
Contributor Author

@doadin doadin commented Jan 12, 2022

@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 Jan 12, 2022
@doadin
Copy link
Contributor Author

@doadin doadin commented Jan 12, 2022

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 issue Jan 13, 2022
@doadin
Copy link
Contributor Author

@doadin doadin commented Jan 15, 2022

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

@glyph
Copy link
Member

@glyph 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 doadin commented Jan 15, 2022

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

Copy link
Member

@adiroiban adiroiban left a comment

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 Feb 7, 2022
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@doadin
Copy link
Contributor Author

@doadin doadin commented Feb 7, 2022

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

Copy link
Member

@adiroiban adiroiban left a comment

Looks good. Many thanks!

@adiroiban adiroiban enabled auto-merge Feb 7, 2022
@@ -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

@glyph glyph Feb 7, 2022

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

@doadin doadin Feb 7, 2022

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

@glyph glyph Feb 7, 2022

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

@glyph glyph Feb 7, 2022

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 adiroiban disabled auto-merge Feb 8, 2022
@adiroiban
Copy link
Member

@adiroiban adiroiban commented Feb 8, 2022

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

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

@doadin doadin commented Feb 8, 2022

@adiroiban @glyph
Thanks!

cas-- added a commit to cas--/Deluge that referenced this issue Feb 9, 2022
Fix for Windows simulate error has been merged and in 22.2.0rc

Ref: twisted/twisted#1679
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