Opened 9 years ago
Closed 8 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 )
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)
Change History (34)
comment:1 Changed 9 years ago by
Keywords: | easy added |
---|---|
Owner: | Itamar Turner-Trauring deleted |
Changed 9 years ago by
Attachment: | journalDeprecation-4298.diff added |
---|
Changed 9 years ago by
Attachment: | deprecateJournal-4298.diff added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|---|
Priority: | normal → highest |
comment:3 Changed 9 years ago by
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 9 years ago by
Priority: | highest → normal |
---|
This is blocked by #4111 and doesn't need a higher priority, apps aren't breaking or anything.
comment:5 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Drew Smathers |
Thanks! Some simple comments:
- Missing docstrings on the test class and test method
- The test will fail if it is run twice in one process. You could either:
- pop it out of sys.modules before the test runs
- 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 9 years ago by
Author: | → amacleod |
---|---|
Branch: | → branches/deprecatepersistedjournal-4298 |
(In [28690]) Branching to 'deprecatepersistedjournal-4298'
comment:9 Changed 9 years ago by
comment:11 Changed 9 years ago by
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:13 Changed 9 years ago by
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 9 years ago by
Status: | new → assigned |
---|
Thanks for the heads-up cyli. I will add a news file next time I'm home and have free time.
comment:15 Changed 9 years ago by
Thanks. Could you also:
- update the copyright in twisted/persisted/journal/init.py
- use
2010
instead of2001-2010
in the new test_journal.py
Merge after that.
comment:16 Changed 9 years ago by
Poke, poke, amacleod: this is almost done! Are you going to take it across the finish line or what? :)
comment:17 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Finally got off my duff and did it: [29264].
comment:18 Changed 9 years ago by
Hi amacleod,
Please read http://twistedmatrix.com/trac/wiki/ReviewProcess before your next trunk commit. :)
comment:19 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:20 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:21 Changed 9 years ago by
Owner: | Allister MacLeod deleted |
---|---|
Status: | reopened → new |
comment:22 Changed 8 years ago by
This was reverted in r29265 because it was missing a news file. Should be easy for anyone to finish up.
comment:23 Changed 8 years ago by
Author: | amacleod → cyli, amacleod |
---|---|
Branch: | branches/deprecatepersistedjournal-4298 → branches/re-deprecate-persisted-journal-4298 |
(In [30451]) Branching to 're-deprecate-persisted-journal-4298'
comment:24 Changed 8 years ago by
comment:27 Changed 8 years ago by
Author: | cyli, amacleod → cyli, amacleod, djfroofy |
---|
comment:28 follow-up: 30 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Ying Li |
Merge after updating the copyright in twisted/test/test_journal
.
comment:29 Changed 8 years ago by
comment:30 Changed 8 years ago by
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 8 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:32 Changed 8 years ago by
Owner: | Ying Li deleted |
---|
I'd rather not spend typing time on this, but it's certainly worth doing.