Skip to content

bpo-1635741: Port time module to multiphase initialization (PEP 489)#19107

Merged
vstinner merged 1 commit intopython:masterfrom
phsilva:bpo-1635741
Mar 23, 2020
Merged

bpo-1635741: Port time module to multiphase initialization (PEP 489)#19107
vstinner merged 1 commit intopython:masterfrom
phsilva:bpo-1635741

Conversation

@phsilva
Copy link
Contributor

@phsilva phsilva commented Mar 22, 2020 •
edited by bedevere-bot
Loading

shihai1991
shihai1991 reviewed Mar 22, 2020
View reviewed changes
Modules/timemodule.c Outdated
Copy link
Member

@shihai1991 shihai1991 Mar 22, 2020

Choose a reason for hiding this comment

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

Use mod or module would be more exact than m ;)

Copy link
Member

@vstinner vstinner Mar 22, 2020

Choose a reason for hiding this comment

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

Yeah, please this PR as an opportunity to replace m with module ;-)

Copy link
Member

@shihai1991 shihai1991 Mar 22, 2020

Choose a reason for hiding this comment

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

I have an impractical idea: Making an review robot to give some suggestions of normative problem automatically ;)

@shihai1991
Copy link
Member

shihai1991 commented Mar 22, 2020

cc @vstinner @corona10

ps: I don't know what happend with the gate of macOS(Looks like it have been broken several days.)

@corona10
Copy link
Member

corona10 commented Mar 22, 2020

@shihai1991
This is the known issue :)
https://bugs.python.org/issue40014

@shihai1991
Copy link
Member

shihai1991 commented Mar 22, 2020

@shihai1991
This is the known issue :)
https://bugs.python.org/issue40014

copy that, thanks ;0

@phsilva phsilva requested a review from vstinner March 23, 2020 01:26
vstinner
vstinner reviewed Mar 23, 2020
View reviewed changes
Modules/timemodule.c Outdated
Copy link
Member

@vstinner vstinner Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

DECREF here is wrong. Replace "goto error" with "return -1" and remove the error label.

cc @corona10

Copy link
Contributor Author

@phsilva phsilva Mar 23, 2020

Choose a reason for hiding this comment

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

I started not wanting to refactor code but looks like a good chance for more cleaning. Thanks, already changed on latest push.

@phsilva phsilva requested a review from vstinner March 23, 2020 18:37
vstinner
vstinner approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Ok, this time it LGTM. Thanks ;-)

@vstinner vstinner merged commit bd409bb into python:master Mar 23, 2020
@phsilva phsilva deleted the bpo-1635741 branch March 24, 2020 07:24
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shihai1991 shihai1991 shihai1991 left review comments

@vstinner vstinner vstinner approved these changes

@abalkin abalkin Awaiting requested review from abalkin

@pganssle pganssle Awaiting requested review from pganssle

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@phsilva @shihai1991 @corona10 @vstinner @the-knights-who-say-ni @bedevere-bot