Ticket #4298 task closed fixed

Opened 4 years ago

Last modified 3 years ago

Deprecate twisted.persisted.journal

Reported by: thijs Owned by:
Priority: normal Milestone:
Component: core Keywords: easy
Cc: thijs, exarkun Branch: branches/re-deprecate-persisted-journal-4298
(diff, github, buildbot, log)
Author: cyli, amacleod, djfroofy Launchpad Bug:

Description (last modified by exarkun) (diff)

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

journalDeprecation-4298.diff Download (1.5 KB) - added by djfroofy 4 years ago.
deprecateJournal-4298.diff Download (0.5 KB) - added by djfroofy 4 years ago.

Change History

1

  Changed 4 years ago by itamar

  • owner itamar deleted
  • keywords easy added

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

Changed 4 years ago by djfroofy

Changed 4 years ago by djfroofy

2

  Changed 4 years ago by djfroofy

  • keywords review added
  • priority changed from normal to highest

3

  Changed 4 years ago by djfroofy

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.

4

  Changed 4 years ago by thijs

  • priority changed from highest to normal

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

5

  Changed 4 years ago by exarkun

  • keywords review removed
  • owner set to djfroofy

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).

6

  Changed 4 years ago by amacleod

  • branch set to branches/deprecatepersistedjournal-4298
  • branch_author set to amacleod

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

7

  Changed 4 years ago by amacleod

Updated changes in [28590:28694].

8

  Changed 4 years ago by amacleod

Er.. I mean [28690:28694].

9

10

  Changed 4 years ago by amacleod

  • keywords review added

Ready for re-review.

11

  Changed 4 years ago by cyli

  • keywords review removed
  • owner changed from djfroofy to amacleod

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.

12

  Changed 4 years ago by amacleod

Incremented deprecated-at version in r28714

13

  Changed 4 years ago by cyli

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).

14

  Changed 4 years ago by amacleod

  • status changed from new to assigned

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

15

  Changed 4 years ago by thijs

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.

16

  Changed 4 years ago by glyph

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

17

  Changed 4 years ago by amacleod

  • status changed from assigned to closed
  • resolution set to fixed

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

18

  Changed 4 years ago by exarkun

Hi amacleod,

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

19

  Changed 4 years ago by exarkun

  • status changed from closed to reopened
  • resolution fixed deleted

20

  Changed 4 years ago by exarkun

  • description modified (diff)

21

  Changed 4 years ago by amacleod

  • owner amacleod deleted
  • status changed from reopened to new

22

  Changed 3 years ago by exarkun

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

23

  Changed 3 years ago by cyli

  • branch changed from branches/deprecatepersistedjournal-4298 to branches/re-deprecate-persisted-journal-4298
  • branch_author changed from amacleod to cyli, amacleod

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

24

  Changed 3 years ago by cyli

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

refs #4298

25

  Changed 3 years ago by cyli

Oops - majority of the changes were in  r30457

26

  Changed 3 years ago by cyli

  • keywords review added

Here are some  build results

27

  Changed 3 years ago by thijs

  • branch_author changed from cyli, amacleod to cyli, amacleod, djfroofy

28

follow-up: ↓ 30   Changed 3 years ago by thijs

  • keywords review removed
  • owner set to cyli

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

29

in reply to: ↑ description   Changed 3 years ago by thijs

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.

30

in reply to: ↑ 28   Changed 3 years ago by cyli

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. :)

31

  Changed 3 years ago by cyli

  • status changed from new to closed
  • resolution set to fixed

(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.

32

  Changed 3 years ago by <automation>

  • owner cyli deleted
Note: See TracTickets for help on using tickets.