Opened 11 years ago

Closed 10 years ago

#4806 defect closed fixed (fixed)

The second time it's used, deprecatedModuleAttribute still warns twice

Reported by: Glyph Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/double-deprecate-4806
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

As discovered in the implementation of #4298, one #4492 was not quite completely fixed.

If you deprecate the attribute module on the package package, the second time you from package import module, you'll get 2 warnings instead of one.

Attachments (1)

deprecate-twice.diff (4.4 KB) - added by Glyph 11 years ago.

Download all attachments as: .zip

Change History (15)

Changed 11 years ago by Glyph

Attachment: deprecate-twice.diff added

comment:1 Changed 11 years ago by Glyph

The attached patch demonstrates the issue - it's a test patch only, no implementation fix yet. I also took the opportunity to fix the fact that trial -u twisted.test.test_deprecate fails on its second run. (Ironically, failing with the exact issue that this ticket describes, because the test wasn't cleaning up after itself properly.)

comment:2 Changed 11 years ago by <automation>

Owner: Glyph deleted

comment:3 Changed 11 years ago by Glyph

The reason this happens is actually obliquely explained already in the comment in _DeprecatedAttribute.get. The first time that the import system comes around to see if the attribute exists, I guess in the C equivalent of a hasattr, to see if the name already exists, it gets an AttributeError and doesn't produce a warning. But in all subsequent calls, when that check succeeds, the call to actually get the name and give it back to user code is a separate __getattribute__ invocation, and both produce a warning.

comment:4 Changed 11 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:5 Changed 11 years ago by Glyph

Author: glyph
Branch: branches/double-deprecate-4806

(In [31744]) Branching to 'double-deprecate-4806'

comment:6 Changed 11 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

Build Results, when they're ready.

The solution in the branch is only heuristically correct, but I figured it wasn't worth going to the trouble of being more correct, since you'd have to do some awful stuff (lots of stack-walking, making it brittle and even less friendly to import hooks). It should reliably work in the default case though.

comment:7 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Glyph

Thanks. Here are some rather nit-picky comments:

  1. The diff was rather difficult to read since it smashed the _InternalState change together with actual logic changes (which are somewhat subtle). It would have been better if this could have been done as two patches, one to just switch to _InternalState, and one on top of that which fixed the bug described in the summary.
  2. Maybe _ModuleProxy.__getattribute__ would be made easier to read by the addition of a comment or two explaining the difference between deprecatedAttribute being None and otherwise.
  3. Two other unit tests in twisted.python.test.test_deprecate still fail when run more than once; this might merit a new ticket.
  4. It might be nice to assert something about the contents of emitted in checkOneWarning, in addition to the length. Or maybe this is redundant with existing tests (but they don't exercise this case !)
  5. It would be nice for PySpaceTestCase to move to a non-test_ module and to get a class docstring. This should make it more clear that it's a piece of re-used test infrastructure, not something particular to the test_modules.py suite alone. test_deprecate.py will actually be the third consumer of this API (counting test_modules.py) now.

Of these points, I am mostly interested in the last, as a matter of improving the factoring and organization of our test suite. Please merge when you are satisfied with the branch with respect to these issues. Also, you might be pleased to know that this works on PyPy.

comment:8 Changed 10 years ago by Glyph

(In [32319]) re #4806 point 4, it is annoying to compute the whole thing (lineno, filename) but at least make sure the emitted list has the vaguest shape of a deprecation warning.

comment:9 Changed 10 years ago by Glyph

(In [32320]) re #4806 review point 5: move PySpaceTestCase to a new home in twisted.python.test.modules_helpers since it is a fixture and not itself a test case, and give it a less ridiculous and obsolete name (TwistedModulesTestCase)

comment:10 Changed 10 years ago by Glyph

To point 1: in the future, I will (as always) endeavor to make my changesets shorter and clearer.

comment:11 Changed 10 years ago by Glyph

(In [32324]) re #4806 point 2, I think this is a bit redundant, but this code is quite hard to follow, so I'll try to make it as clear as possible.

comment:12 Changed 10 years ago by Glyph

Once more, with feeling.

comment:13 Changed 10 years ago by Glyph

For point 3, I filed #5197.

comment:14 Changed 10 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [32325]) Merge double-deprecate-4806: fix double-warning about module deprecations.

Author: glyph

Reviewer: exarkun

Fixes: #4806

twisted.python.deprecate.deprecatedModuleAttribute no longer spuriously warns twice when used to deprecate a module within a package. This should make it easier to write unit tests for deprecated modules.

Note: See TracTickets for help on using tickets.