gh-80793: Edit unittest.rst on SetUpClass calls #12798

vrpolakatcisco · 2019-04-12T12:23:15Z

Signed-off-by: Vratko Polak [email protected]

Issue: Unittest document is not clear on SetUpClass calls #80793

vrpolakatcisco · 2019-04-12T12:32:18Z

bedevere/news — News entry found in Misc/NEWS.d

I fail at blurb. How can I fix?

Misc/NEWS.d/next/Documentation/2019-04-12-12-30-08.bpo-36612.8AGXVv.rst

MakDon · 2019-06-10T02:29:05Z

Hi, @vrpolakatcisco ,it is no need to add a 'skip news' in Misc/NEWS.d. A core dev would review your commit and add a 'skip news' tag on the PR.
I haven't checked the document yet. I will check it later.

Doc/library/unittest.rst

vrpolakatcisco · 2019-07-16T14:51:24Z

vrpolakatcisco wants to merge 2 commits into python:master from vrpolakatcisco:fixtures_doc

Well, my plan was to do a clean rebase. Not sure what to do with the "sync" merge commit.

JulienPalard

Thanks for trying to enhance the documentation! I propose to simplify further, as it's only normal rules.

Doc/library/unittest.rst

JulienPalard · 2019-09-11T15:22:08Z

Doc/library/unittest.rst

Same, what are you trying to clarify? As said in b.p.o normal inheritance rules apply. I think we should be explicit about inheritance only when "anormal rule apply", not every time "normal" rules apply.

The original formulation
"These must be implemented as class methods"
sounds like normal rules do not apply for some reason, and each class needs to have an implementation (even if it only calls ancestor implementation).

sounds like normal rules do not apply for some reason

Yes, it tells it's not a method but a class method, it reminds you not to forgot to add the @classmethod, the original wording is OK.

and each class needs to have an implementation (even if it only calls ancestor implementation)

Can you proove it with an example?

JulienPalard · 2019-09-11T15:23:57Z

Doc/library/unittest.rst

Same here, let's not be explicit about this being normal (if we're explicit here about the inheritance working normally, why not adding it everywhere else on the doc? No.)

The original formulation
"you must call up to them yourself"
once again sounds like normal rules do not apply for some reason, and you have to make the call explicit, instead of relying on inheritance.

if we're explicit here about the inheritance working normally, why not adding it everywhere else on the doc?

I agree that confirming something is normal creates doubts on other (not explicitly confirmed things).

I am going to propose different edits, focusing on just removing the "wrong sounding" formulations.

once again sounds like normal rules do not apply for some reason

I agree.

and you have to make the call explicit, instead of relying on inheritance

Inheritance never "call implicitly", as long as you re-implement the method, you have to call the parent implementation manually. But if you don't implement a method, the parent one is just inherited and no manual actions are needed. Only normal rules here.

bedevere-bot · 2019-09-11T15:25:57Z

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.

methane · 2019-09-12T11:52:34Z

FYI, merge master is enough because we use "Squash and merge" workflow.
Merge commits created by "merge master" command are not merged into the master branch.

JulienPalard · 2019-09-12T12:49:02Z

Doc/library/unittest.rst

Why removing the "if there is one"? It was right:

tearDownClass = getattr(previousClass, 'tearDownClass', None) if tearDownClass is not None: ...

I don't see any problem with this paragraph, and it looks not correlated with bpo-36612.

JulienPalard · 2019-09-12T12:50:23Z

Doc/library/unittest.rst

Looks unrelated to the issue, and I don't see how the new wording helps.

JulienPalard · 2019-09-12T12:50:41Z

Doc/library/unittest.rst

I prefer the original wording.

Not sure why module functions have "should" and class methods have "must", but I think I can make do edits also without affecting these two lines.

JulienPalard · 2019-09-12T12:53:39Z

Doc/library/unittest.rst

This sentence is right, the implementations are present but empty. But now without context I don't see the point of mentioning it, why not removing it all?

I will expand in next version.

Honestly this still doesn't sound very useful to me. The new text doesn't really answer the question in the original bug report.

The previous formulations could suggest that some special rules apply for the class methods as opposed to the normal inheritance rules. The formulations are tweaked by this patch, hinting the normal inheritance rules indeed apply. Sentences describing module functions are also tweaked, to signify the logic (almost) the same as for class methods. Signed-off-by: Vratko Polak <[email protected]>

vrpolakatcisco · 2019-09-13T11:57:49Z

Doc/library/unittest.rst

 ``tearDownModule`` are run.

+The abovementioned calls are only executed,
+if the corresponding method or function is found.


Choosing "method is found", instead of "method exists" or "class has a method", in order to hint method resolution order is in effect.

vrpolakatcisco · 2019-09-13T11:58:51Z

Doc/library/unittest.rst

-then you must call up to them yourself. The implementations in
-:class:`TestCase` are empty.
+The implementations in :class:`TestCase` are present, but empty.
+That means the implemenation is executed for classes descending from


Choosing "descending" instead of "derived" or "child", to suggest the inheritance graphs can be complicated.

vrpolakatcisco · 2019-09-13T12:00:47Z

Doc/library/unittest.rst

-:class:`TestCase` are empty.
+The implementations in :class:`TestCase` are present, but empty.
+That means the implemenation is executed for classes descending from
+:class:`TestCase`, but the implementation does not do anything,


Empty implementation executed means no special handling is there to interfere with normal inheritance rules.

willingc · 2024-10-31T19:52:44Z

This PR has been languishing. I'm going to go ahead and close the PR and associated issue.

Thanks @vrpolakatcisco for the report and the PR.

the-knights-who-say-ni added the CLA not signed label Apr 12, 2019

bedevere-bot added the docs Documentation in the Doc dir label Apr 12, 2019

This comment was marked as resolved.

Sign in to view

bedevere-bot added the awaiting review label Apr 12, 2019

This comment was marked as resolved.

Sign in to view

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Apr 12, 2019

This comment was marked as resolved.

Sign in to view

MakDon suggested changes Jun 10, 2019

View reviewed changes

Misc/NEWS.d/next/Documentation/2019-04-12-12-30-08.bpo-36612.8AGXVv.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Jun 10, 2019

MakDon reviewed Jun 10, 2019

View reviewed changes

Doc/library/unittest.rst Outdated Show resolved Hide resolved

vrpolakatcisco force-pushed the fixtures_doc branch from d3b7c16 to 1afa8fa Compare July 16, 2019 14:47

JulienPalard added the skip news label Sep 11, 2019

JulienPalard requested changes Sep 11, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 11, 2019

vrpolakatcisco force-pushed the fixtures_doc branch from 1afa8fa to c3e84ec Compare September 12, 2019 11:24

vrpolakatcisco requested review from 1st1, abalkin, asvetlov, brettcannon, encukou, ericsnowcurrently, ericvsmith and ezio-melotti as code owners September 12, 2019 11:24

zware requested review from JulienPalard and removed request for abalkin, ericsnowcurrently, ericvsmith, ezio-melotti, gvanrossum, ilevkivskyi, ncoghlan, pablogsal, pganssle, rhettinger and terryjreedy September 12, 2019 11:42

JulienPalard requested changes Sep 12, 2019

View reviewed changes

vrpolakatcisco force-pushed the fixtures_doc branch from c2526ff to bb63073 Compare September 13, 2019 11:54

vrpolakatcisco commented Sep 13, 2019

View reviewed changes

vrpolakatcisco requested a review from JulienPalard October 4, 2019 14:57

Merge branch 'main' into fixtures_doc

b331498

vrpolakatcisco mannequin mentioned this pull request Apr 10, 2022

Unittest document is not clear on SetUpClass calls #80793

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into fixtures_doc

1a54ee5

AlexWaygood changed the title ~~bpo-36612: Edit unittest.rst on SetUpClass calls~~ gg-80793: Edit unittest.rst on SetUpClass calls Oct 30, 2022

AlexWaygood changed the title ~~gg-80793: Edit unittest.rst on SetUpClass calls~~ gh-80793: Edit unittest.rst on SetUpClass calls Oct 30, 2022

willingc added the pending The issue will be closed if no feedback is provided label Oct 31, 2024

willingc closed this Oct 31, 2024

AA-Turner removed the pending The issue will be closed if no feedback is provided label Apr 6, 2025

Uh oh!

Uh oh!

gh-80793: Edit unittest.rst on SetUpClass calls #12798

gh-80793: Edit unittest.rst on SetUpClass calls #12798

Uh oh!

Conversation

vrpolakatcisco commented Apr 12, 2019 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

vrpolakatcisco commented Apr 12, 2019

Uh oh!

This comment was marked as resolved.

Uh oh!

MakDon commented Jun 10, 2019

Uh oh!

Uh oh!

vrpolakatcisco commented Jul 16, 2019

Uh oh!

JulienPalard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 11, 2019

Uh oh!

methane commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willingc commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

vrpolakatcisco commented Apr 12, 2019 •

edited by AlexWaygood

Loading