bpo-36782: Created C API wrappers and added missing tests for functions in the PyDateTimeAPI. by edison12a · Pull Request #13088 · python/cpython

edison12a · 2019-05-04T21:22:16Z

https://bugs.python.org/issue36782

pganssle

Thank you for this contribution! I'm very excited to have these added to the datetime test suite!

The Python portion of the code looks good to me, but I have some formatting notes on the C functions.

pganssle · 2019-05-04T21:26:15Z

Modules/_testcapimodule.c

-    {"test_incref_doesnt_leak", test_incref_doesnt_leak,         METH_NOARGS},
-    {"test_xdecref_doesnt_leak",test_xdecref_doesnt_leak,        METH_NOARGS},
-    {"test_decref_doesnt_leak", test_decref_doesnt_leak,         METH_NOARGS},
+    {"raise_exception", raise_exception, METH_VARARGS},


Are these formatting changes deliberate? I know some editors will be somewhat aggressive with making formatting changes.

I think it would be best to leave the original formatting intact, if only to make the diff easier to review.

Modules/_testcapimodule.c

…bpo-36782

Misc/NEWS.d/next/Tests/2019-05-04-21-25-19.bpo-36782.h3oPIb.rst

edison12a · 2019-05-04T21:43:19Z

I have made the changes that you requested. @pganssle Please review.

pganssle

Looks good to me! Thanks @SimiCode!

matrixise

Thank you for your contribution, please read my comments with @pganssle and add you to the Misc/ACKS file.

Lib/test/datetimetester.py

matrixise · 2019-05-06T14:03:28Z

Lib/test/datetimetester.py

+            with self.subTest(macro=macro):
+                d = _testcapi.get_date_fromdate(year, month, day, macro)
+
+                self.assertEqual(d, date(1993, 8, 26))


and because I don't like the duplicated code, you could use that.

self.assertEqual(d, date(year, month, day))

I think you could also compute one time the expected date. and because I like to be explicit...

def test_date_from_date(self): expected_date = date(1993, 8, 26) for macro in (0, 1): with self.subTest(macro=macro): new_date = _testcapi.get_date_fromdate(expected_date.year, expected_date.month, expected_date.day, macro) self.assertEqual(new_date, expected_date)

@pganssle ?

bedevere-bot · 2019-05-06T14:06:46Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

edison12a · 2019-05-06T17:39:44Z

I have made the requested changes; please review again

bedevere-bot · 2019-05-06T17:39:51Z

Thanks for making the requested changes!

@matrixise: please review the changes made to this pull request.

matrixise · 2019-05-06T19:13:26Z

Misc/NEWS.d/next/Tests/2019-05-04-21-25-19.bpo-36782.h3oPIb.rst

@@ -0,0 +1,2 @@
+Created a test C API wrapper for PyDate_FromDate and added a test for PyDate_FromDate. 


@pganssle Could you help @SimiCode for the blurb entry? Add tests for the C-API wrapper of PyDate_FromDate ? I am not a native speaker. or Add tests for PyDate_FromDate

Talked to Edison in the room, he will have an updated blurb that I have looked at. The current one is grammatically fine but could be tightened up and could use RST directives.

bedevere-bot · 2019-05-06T19:13:33Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pganssle

New changes look good to me.

vstinner · 2019-05-10T01:39:03Z

@pganssle: Would you mind to review this PR again? @SimiCode updated it. Once you approve it, I'm ok to review it to maybe merge it ;-)

pganssle

I've suggested changing all the int macro = 0 to int macro; now that macro is required, but otherwise this PR looks good and we can merge it.

Modules/_testcapimodule.c

change all the int macro = 0 to int macro; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

change all the `int macro = 0` to `int macro`; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

edison12a · 2019-05-11T23:58:03Z

@vstinner @pganssle
I have added the suggested changes. Please review.

matrixise

Thank you for your contribution. 🎉

pganssle

LGTM! Thanks Edison!

vstinner · 2019-05-17T18:30:12Z

Well done @SimiCode, modifying C code and tests is not trivial!

bedevere-bot · 2019-05-17T20:28:44Z

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

vstinner · 2019-05-17T20:29:22Z

Crap. I modified the commit message, but the merge failed. I click on Retry, and then it lost all my changes on the commit message :-(

vstinner · 2019-05-17T20:30:36Z

@pganssle: Do we need to backport the change to 3.7?

pganssle · 2019-05-17T20:36:40Z

@vstinner It only adds tests, so while I don't think it would hurt to backport the change to 3.7, it's not really necessary.

miss-islington · 2019-05-17T21:43:54Z

Thanks @SimiCode for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2019-05-27T23:33:49Z

Thanks @SimiCode for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2019-05-27T23:33:52Z

Sorry, @SimiCode and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 98ff4d5fb6a9d01b0176b7786db61346952e5295 3.7

created a c API wrapper for pyDate_FromDate and added the test

d9945c2

the-knights-who-say-ni added the CLA signed label May 4, 2019

bedevere-bot added the awaiting review label May 4, 2019

📜🤖 Added by blurb_it.

f1573d6

pganssle requested changes May 4, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 4, 2019

SimiCode added 2 commits May 5, 2019 00:35

fixed auto-alignment by vscode

0284ced

Merge branch 'bpo-36782' of https://github.com/SimiCode/cpython into …

8cca15f

…bpo-36782

pganssle reviewed May 4, 2019

View reviewed changes

Misc/NEWS.d/next/Tests/2019-05-04-21-25-19.bpo-36782.h3oPIb.rst Outdated Show resolved Hide resolved

SimiCode and others added 2 commits May 5, 2019 00:40

made changes as per PEP7

d9ecbf5

Update 2019-05-04-21-25-19.bpo-36782.h3oPIb.rst

7967114

pganssle approved these changes May 4, 2019

View reviewed changes

matrixise requested changes May 6, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label May 6, 2019

bedevere-bot added the awaiting changes label May 6, 2019

SimiCode added 2 commits May 6, 2019 18:10

Refactored code as per requested changes

f6b3ab8

Remove Whitespace to Fix failed travis build

3b2095d

bedevere-bot added awaiting change review and removed awaiting changes labels May 6, 2019

matrixise requested changes May 6, 2019

View reviewed changes

bedevere-bot removed the awaiting change review label May 6, 2019

bedevere-bot added the awaiting changes label May 6, 2019

pganssle approved these changes May 6, 2019

View reviewed changes

Update 2019-05-04-21-25-19.bpo-36782.h3oPIb.rst

f6d590f

Re-arrange the order of unit-test args

6df027e

pganssle approved these changes May 10, 2019

View reviewed changes

edison12a and others added 6 commits May 11, 2019 16:53

Removed the need to initialize macro

48f7e03

change all the int macro = 0 to int macro; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

Removed the need to initialize macro

7e91753

change all the `int macro = 0` to `int macro`; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

Removed the need to initialize macro

588d95a

change all the `int macro = 0` to `int macro`; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

Removed the need to initialize macro

eb3e060

change all the `int macro = 0` to `int macro`; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

Removed the need to initialize macro

1e4db07

change all the `int macro = 0` to `int macro`; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

Removed the need to initialize macro

a9c7623

change all the `int macro = 0` to `int macro`; now that macro is required Co-Authored-By: Paul Ganssle <pganssle@users.noreply.github.com>

matrixise approved these changes May 15, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels May 15, 2019

pganssle approved these changes May 15, 2019

View reviewed changes

vstinner approved these changes May 17, 2019

View reviewed changes

vstinner merged commit 98ff4d5 into python:master May 17, 2019

bedevere-bot removed the awaiting merge label May 17, 2019

vstinner added the needs backport to 3.7 label May 17, 2019

vstinner added needs backport to 3.7 and removed needs backport to 3.7 labels May 27, 2019

miss-islington assigned vstinner May 27, 2019

edison12a deleted the bpo-36782 branch June 6, 2020 13:04

		@@ -0,0 +1,2 @@
		Created a test C API wrapper for PyDate_FromDate and added a test for PyDate_FromDate.

Uh oh!

Conversation

edison12a commented May 4, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

pganssle May 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edison12a commented May 4, 2019

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

matrixise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matrixise May 6, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 6, 2019

Uh oh!

edison12a commented May 6, 2019

Uh oh!

bedevere-bot commented May 6, 2019

Uh oh!

matrixise May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle May 6, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 6, 2019

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 10, 2019

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edison12a commented May 11, 2019

Uh oh!

matrixise left a comment

Choose a reason for hiding this comment

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 17, 2019

Uh oh!

bedevere-bot commented May 17, 2019

Uh oh!

vstinner commented May 17, 2019

Uh oh!

vstinner commented May 17, 2019

Uh oh!

pganssle commented May 17, 2019

Uh oh!

miss-islington commented May 17, 2019

Uh oh!

miss-islington commented May 27, 2019

Uh oh!

miss-islington commented May 27, 2019

Uh oh!

Reviewers

Assignees

edison12a commented May 4, 2019 •

edited by bedevere-bot

Loading

matrixise May 6, 2019 •

edited

Loading