Opened 12 years ago

Closed 11 years ago

#4298 task closed fixed (fixed)

Deprecate twisted.persisted.journal

Reported by: Thijs Triemstra Owned by:
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra, Jean-Paul Calderone Branch: branches/re-deprecate-persisted-journal-4298
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli, amacleod, djfroofy

Description (last modified by Jean-Paul Calderone)

twisted.persisted.journal is one of the not-quite-reliable storage systems still floating around from Twisted's sordid past. No one has any intention of finishing it (clearly, as it's been 4 years since its last change), and by current standards we probably wouldn't want it in Twisted anyway due to how far afield from Twisted's primary goals it is.

It should be deprecated so we can remove it in a few more releases.

Attachments (2)

journalDeprecation-4298.diff (1.5 KB) - added by Drew Smathers 12 years ago.
deprecateJournal-4298.diff (548 bytes) - added by Drew Smathers 12 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 12 years ago by Itamar Turner-Trauring

Keywords: easy added
Owner: Itamar Turner-Trauring deleted

I'd rather not spend typing time on this, but it's certainly worth doing.

Changed 12 years ago by Drew Smathers

Changed 12 years ago by Drew Smathers

Attachment: deprecateJournal-4298.diff added

comment:2 Changed 12 years ago by Drew Smathers

Keywords: review added
Priority: normalhighest

comment:3 Changed 12 years ago by Drew Smathers

Unit test from first patch is stupid and causes breakage in test suite anyhow. I'm not sure how to correctly test deprecation warning in module import or if this is even necessary.

comment:4 Changed 12 years ago by Thijs Triemstra

Priority: highestnormal

This is blocked by #4111 and doesn't need a higher priority, apps aren't breaking or anything.

comment:5 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Drew Smathers

Thanks! Some simple comments:

  1. Missing docstrings on the test class and test method
  2. The test will fail if it is run twice in one process. You could either:
    1. pop it out of sys.modules before the test runs
    2. deprecate the journal attribute of twisted.persisted instead, using deprecateModuleAttribute (although I've never done it this way and I don't know what will happen).

comment:6 Changed 12 years ago by Allister MacLeod

Author: amacleod
Branch: branches/deprecatepersistedjournal-4298

(In [28690]) Branching to 'deprecatepersistedjournal-4298'

comment:7 Changed 12 years ago by Allister MacLeod

Updated changes in [28590:28694].

comment:8 Changed 12 years ago by Allister MacLeod

Er.. I mean [28690:28694].

comment:10 Changed 12 years ago by Allister MacLeod

Keywords: review added

Ready for re-review.

comment:11 Changed 12 years ago by Ying Li

Keywords: review removed
Owner: changed from Drew Smathers to Allister MacLeod

Deprecation should be "as of 10.1", since the current version of Twisted is 10.0.

Everything else seems to be great though, and the tests pass. This can probably be merged into trunk after that change.

comment:12 Changed 12 years ago by Allister MacLeod

Incremented deprecated-at version in r28714

comment:13 Changed 12 years ago by Ying Li

Sorry, I should have spotted this last time, but it's missing an entry in the relevant topfile directory listing what change this branch will make (see http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles).

comment:14 Changed 12 years ago by Allister MacLeod

Status: newassigned

Thanks for the heads-up cyli. I will add a news file next time I'm home and have free time.

comment:15 Changed 12 years ago by Thijs Triemstra

Thanks. Could you also:

  • update the copyright in twisted/persisted/journal/init.py
  • use 2010 instead of 2001-2010 in the new test_journal.py

Merge after that.

comment:16 Changed 12 years ago by Glyph

Poke, poke, amacleod: this is almost done! Are you going to take it across the finish line or what? :)

comment:17 Changed 11 years ago by Allister MacLeod

Resolution: fixed
Status: assignedclosed

Finally got off my duff and did it: [29264].

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

Hi amacleod,

Please read http://twistedmatrix.com/trac/wiki/ReviewProcess before your next trunk commit. :)

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

Resolution: fixed
Status: closedreopened

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

Description: modified (diff)

comment:21 Changed 11 years ago by Allister MacLeod

Owner: Allister MacLeod deleted
Status: reopenednew

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

This was reverted in r29265 because it was missing a news file. Should be easy for anyone to finish up.

comment:23 Changed 11 years ago by Ying Li

Author: amacleodcyli, amacleod
Branch: branches/deprecatepersistedjournal-4298branches/re-deprecate-persisted-journal-4298

(In [30451]) Branching to 're-deprecate-persisted-journal-4298'

comment:24 Changed 11 years ago by Ying Li

(In [30458]) Changed some spacing and whitespace to comply with coding standard.

refs #4298

comment:25 Changed 11 years ago by Ying Li

Oops - majority of the changes were in r30457

comment:26 Changed 11 years ago by Ying Li

Keywords: review added

Here are some build results

comment:27 Changed 11 years ago by Thijs Triemstra

Author: cyli, amacleodcyli, amacleod, djfroofy

comment:28 Changed 11 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to Ying Li

Merge after updating the copyright in twisted/test/test_journal.

comment:29 in reply to:  description Changed 11 years ago by Thijs Triemstra

Replying to thijs:

It should be deprecated so we can remove it in a few more releases.

Opened #4805 for removing it in a future release.

comment:30 in reply to:  28 Changed 11 years ago by Ying Li

Replying to thijs:

Merge after updating the copyright in twisted/test/test_journal.

I did so in r30458. It's just when I entered the commit message for r30457, I used the word "ref" instead of "refs", so trac didn't pick it up. Hence I linked to r30457 manually, but r30458 has some additional changes.

Anyway, thanks for the review. Merging. :)

comment:31 Changed 11 years ago by Ying Li

Resolution: fixed
Status: newclosed

(In [30460]) Merge re-deprecate-persisted-journal-4298: Deprecate twisted.persisted.journal

Author: djfroofy, amacleod, cyli

Reviewer: thijs, exarkun, cyli

Fixes: #4298

Re-deprecate twisted.persisted.journal as of 11.0, after reversion (of deprecation as of 10.1) due to lack of newsfile.

comment:32 Changed 11 years ago by <automation>

Owner: Ying Li deleted
Note: See TracTickets for help on using tickets.