GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-36025: Restore original function API for PyDate_FromTimestamp by pganssle · Pull Request #11922 · python/cpython

Conversation

@pganssle

@pganssle pganssle commented Feb 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Per PyO3/pyo3#352, I believe we've broken the C API for PyDateTimeAPI->Date_FromTimestamp. As a test (and to make sure we don't do it again), I've developed these tests. They are currently passing on 3.7 and they should be failing on master / Python 3.8 since we're passing a tuple to a function that strictly takes int or float.

Once I verify that these tests are indeed failing on CI, (they are anomalously passing in my local directory against master), I will also fix the underlying problem.

We don't need a news item for this, as it fixes a regression that was only ever released in alpha.

https://bugs.python.org/issue36025

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 18, 2019
@bedevere-bot bedevere-bot added the awaiting review label Feb 18, 2019
@pganssle

pganssle commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

Ah, found the reason why it's anomalously passing - I accidentally shadowed the failing test with a passing one. 😅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 6959922 to 0aadc17 Compare February 18, 2019 17:40
@pganssle pganssle mentioned this pull request Feb 18, 2019
Python 3.8 breaks datetime test PyO3/pyo3#352
Closed
@pganssle pganssle changed the title bpo-36025: Add tests for datetime fromtimestamp API bpo-36025: Restore original function API for PyDate_FromTimestamp Feb 18, 2019
serhiy-storchaka
serhiy-storchaka reviewed Feb 18, 2019
View reviewed changes
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Feb 18, 2019

Copy link
Copy Markdown
Member

Since it fixes a regression in released version, I think it should be documented with a news entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle

pganssle commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

@serhiy-storchaka To be clear, this is only a regression on the 3.8 branch. I didn't think alpha versions counted as "released versions", but if they do I can add a news entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 760634c to f268f7a Compare February 18, 2019 20:50
@pganssle

pganssle commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

@serhiy-storchaka Thanks for the review, I made all the suggested changes, except I haven't added a NEWS entry yet.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Feb 19, 2019
View reviewed changes
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch 3 times, most recently from ded3ef2 to d9069e6 Compare February 19, 2019 19:43
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from d9069e6 to 4040647 Compare February 19, 2019 19:44
@pganssle

pganssle commented Feb 27, 2019

Copy link
Copy Markdown
Member Author

@serhiy-storchaka Does this look good? Can we merge or are we waiting on something else?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 4040647 to 8a597f8 Compare February 27, 2019 14:26
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 8a597f8 to 314f828 Compare March 8, 2019 16:09
@pganssle

pganssle commented Mar 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Just realized I never did this for this issue:

CC @abalkin
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abalkin
abalkin self-requested a review March 8, 2019 20:42
abalkin
abalkin approved these changes Mar 8, 2019
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 8, 2019
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 314f828 to 016deca Compare March 17, 2019 19:13
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 016deca to 8f8f617 Compare March 17, 2019 19:16
@pganssle

pganssle commented Mar 17, 2019

Copy link
Copy Markdown
Member Author

The first force push since approval was an accident - included a What's New entry that was intended for another PR. The second one corrects that mistake and rebases this against the current master.

@abalkin @ambv Can we merge this before the next alpha release? It would be good to get at least the pyo3 3.8-dev build job working again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 8f8f617 to 2a231bc Compare March 17, 2019 19:45
serhiy-storchaka
serhiy-storchaka approved these changes Mar 18, 2019
View reviewed changes
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 2a231bc to 2cd3128 Compare April 13, 2019 00:14
ZackerySpytz
ZackerySpytz reviewed Apr 13, 2019
View reviewed changes
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
ZackerySpytz
ZackerySpytz reviewed Apr 13, 2019
View reviewed changes

@ZackerySpytz ZackerySpytz left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There are reference leaks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from 9c07733 to a61ec40 Compare April 13, 2019 20:15
@pganssle

pganssle commented Apr 13, 2019

Copy link
Copy Markdown
Member Author

@ZackerySpytz Thanks for the review! You are right about the refcounts, I have removed them. I guess PyTuple_Pack "steals" the existing reference to both its arguments, so I don't have to increase or decrease the reference count, just pass it into the tuple. I've checked that the reference count is unchanged before and after calling get_datetime_fromtimestamp and it seems it is now!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from a61ec40 to d971844 Compare April 20, 2019 18:31
@pganssle

pganssle commented Apr 23, 2019

Copy link
Copy Markdown
Member Author

@abalkin @serhiy-storchaka Next alpha release is coming up soon. Anything else you'd like me to do before merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pganssle
bpo-36025: Fix PyDate_FromTimestamp API (pythonGH-11922)
ffe34ed 
In the process of converting the date.fromtimestamp function to use
argument clinic in pythonGH-8535, the C API for PyDate_FromTimestamp was
inadvertently changed to expect a timestamp object rather than an
argument tuple.

This PR fixes this backwards-incompatible change by adding a new wrapper
function for the C API function that unwraps the argument tuple and
passes it to the underlying function.

This PR also adds tests for both PyDate_FromTimestamp and
PyDateTime_FromTimestamp to prevent any further regressions.

bpo-36025
@pganssle
pganssle force-pushed the add_fromtimestamp_test_master branch from d971844 to ffe34ed Compare April 27, 2019 19:16
berkerpeksag
berkerpeksag approved these changes Apr 27, 2019
View reviewed changes
@berkerpeksag
berkerpeksag merged commit 4d8c8c0 into python:master Apr 27, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes
@abalkin abalkin abalkin approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@pganssle @serhiy-storchaka @berkerpeksag @abalkin @ZackerySpytz @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.