Opened 9 years ago
Closed 7 years ago
#6395 task closed fixed (fixed)
Deprecate twisted.words.protocols.msn
Reported by: | therve | Owned by: | hawkowl |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | words | Keywords: | easy |
Cc: | Ralph Meijer, Thijs Triemstra | Branch: |
branches/deprecate-msn-6395-2
branch-diff, diff-cov, branch-cov, buildbot |
Author: | hawkowl |
Description
MSN is shutting down the service, I don't think the client code has any usefulness by itself. We should remove the associated example too.
Attachments (7)
Change History (41)
comment:1 Changed 9 years ago by
Cc: | Ralph Meijer added |
---|
Changed 9 years ago by
Attachment: | deleted_msn.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|
comment:3 follow-up: 4 Changed 9 years ago by
Cc: | Thijs Triemstra added |
---|
Hi your patch seems to be incorrect. Why is this breaking the compatibility policy by not deprecating it first?
comment:4 Changed 9 years ago by
Keywords: | review removed |
---|
Yes you are right. Sorry I was unaware about this. Can you tell me how to deprecate a module? I've looked up in http://twistedmatrix.com/trac/browser/tags/releases/twisted-12.3.0/twisted/python/deprecate.py But didn't make out much out of it.
Changed 9 years ago by
Attachment: | deprecated_msn.patch added |
---|
Deprecated twisted.words.protocols.msn
comment:5 Changed 9 years ago by
Keywords: | review added |
---|
comment:6 Changed 9 years ago by
Keywords: | review removed |
---|
comment:7 Changed 9 years ago by
Keywords: | review added |
---|
comment:8 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Indradhanush Gupta |
Thanks for the patch!
- The comment is unnecessary, it's pretty clear what's going on.
- Please run diff at the root of the Twisted checkout, so it's clear which file the patch applies to.
- All code changes to Twisted need tests.
twisted/conch/test/test_insults.py
should provide an example of how to test deprecation of modules. - The patch should include a news file (https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles).
Please address these points and resubmit your patch.
Changed 9 years ago by
Attachment: | deprecated_msn_added_corrections.patch added |
---|
Added NEWS File and tests to deprecate twisted.words.protocols.msn
comment:10 Changed 9 years ago by
Owner: | Indradhanush Gupta deleted |
---|
comment:11 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Indradhanush Gupta |
- Did you try running the tests? The code fails to run for me. (But see below)
- There isn't a warning emitted
`
#!python
from twisted.words.protocols.msn import NotificationClient
`
but there probably should be.
twisted/protocols/telnet.py
has an example of how to emit a deprecation warning that handles both that and
`
#!python
from twisted.words.protocols import msn
`
You should have a test for both cases.
- Calls to
flushWarnings
should pass the function that is to be blamed by the warning (in this casetest_msn
.- Incidentally, that test should probably be something like
test_msnDeprecated
, and the docstring could be more specific:When importing .... a L{DeprecationWarning} is reported
. (#6301 has some comments on how to write test docstrings)
- Incidentally, that test should probably be something like
Changed 9 years ago by
Attachment: | deprecated_msn_added_corrections_2.patch added |
---|
Made changes according to review points. But trial twisted generates 2 errors.
Changed 9 years ago by
Attachment: | deprecated_msn_added_corrections_2.2.patch added |
---|
Made changes according to review points. But trial twisted generates 2 errors.
comment:12 Changed 9 years ago by
Keywords: | review added |
---|
Made some changes according to suggestions. But trial twisted generates the following errors:
[ERROR] Traceback (most recent call last): File "/home/dhanush/Twisted 12.3.0-original/twisted/mail/test/test_mail.py", line 1937, in _cbProcessAlias lines = file('process.alias.out').readlines() exceptions.IOError: [Errno 2] No such file or directory: 'process.alias.out' twisted.mail.test.test_mail.ProcessAliasTestCase.test_processAlias =============================================================================== [ERROR] Traceback (most recent call last): File "/home/dhanush/Twisted 12.3.0-original/twisted/words/test/test_msn.py", line 545, in test_msnDeprecated self.ensureDeprecated("twisted.words.protocols.msn is deprecated " File "/home/dhanush/Twisted 12.3.0-original/twisted/words/test/test_msn.py", line 535, in ensureDeprecated self.assertIdentical(warnings[0]['category'], DeprecationWarning) exceptions.IndexError: list index out of range twisted.words.test.test_msn.Deprecations.test_msnDeprecated
Error 1 looks to be unrelated to this patch. Couldn't resolve Error 2. Awaiting some advice.
comment:13 Changed 9 years ago by
Owner: | Indradhanush Gupta deleted |
---|
comment:14 Changed 9 years ago by
Owner: | set to Tom Prince |
---|
I am interested in participating in GSoC 13 under Twisted. itamar suggested to assign it to you.
comment:15 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to Indradhanush Gupta |
- The first error is unrelated to this ticket, and is due to the space in your path. (There is already a ticket about addressing this)
- Based on the path I see there, it also appears that you are working against a tarball from 12.3. If you are planning on contributing to twisted, you should really be working from a source checkout see TwistedDevelopment).
- The error you are getting is because there wasn't a warning emitted *during* the test. This is because the module has already been imported, and so the the import statment isn't actually running the code that triggers the deprecation warning.
- As therve suggested on irc, have a look at
twisted/internet/test/test_tls.py
for an example of how to ensure that the code triggering the warning gets run. - Putting the
assertEqual(len(warnings), 1)
at the begining, rather than the end, might have made the error easier to understand: it would have said that thelen(warnings)
is 0 not 1, rather then having to realise thatIndexError
means thatwarnings
isnt long enough.
- As therve suggested on irc, have a look at
Changed 8 years ago by
Attachment: | 6395.patch added |
---|
Completing dhanush's work. Tests have been run against the patch and they pass. Also, the warning is now raised.
comment:16 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Indradhanush Gupta deleted |
comment:17 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Summary: | Delete twisted.words.protocols.msn → Deprecate twisted.words.protocols.msn |
Thanks for your patch.
- the version nr should be bumped to 14.1
- there's still an example in [source:trunk/docs/words/examples/msn_example.py] that needs to be removed
Changed 8 years ago by
Attachment: | 6395-2.patch added |
---|
Building on thijs' comments. Attaching a tested patch.
comment:18 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
comment:19 Changed 8 years ago by
Owner: | set to hawkowl |
---|
comment:20 Changed 8 years ago by
Author: | → hawkowl |
---|---|
Branch: | → branches/deprecate-msn-6395 |
(In [43363]) Branching to deprecate-msn-6395.
comment:23 follow-up: 24 Changed 8 years ago by
there's still an example in trunk/docs/words/examples/msn_example.py that needs to be removed
This is incorrect. The standing policy is to remove the documentation when the feature is removed. Not to remove the documentation when the feature is deprecated.
comment:24 follow-up: 25 Changed 8 years ago by
Replying to exarkun:
This is incorrect. The standing policy is to remove the documentation when the feature is removed. Not to remove the documentation when the feature is deprecated.
I didn't know this was part of policy, and I can't find it in the Documentation standard or Compatibility Policy -- should we add it, or is it somewhere else?
comment:25 follow-up: 26 Changed 8 years ago by
Replying to hawkowl:
Replying to exarkun:
This is incorrect. The standing policy is to remove the documentation when the feature is removed. Not to remove the documentation when the feature is deprecated.
I didn't know this was part of policy, and I can't find it in the Documentation standard or Compatibility Policy -- should we add it, or is it somewhere else?
I'm not sure if it is written down anywhere (if it isn't it should be). The reasoning is, that until the code is gone, people may be using it and so we should keep existing documentation about it.
- Perhaps they inherited a codebase that is already using it, and they need to figure it out before removing the usage
- In a case like this, where we are removing functionality, they may have use of it, even if we aren't going to continue supporting it.
In general, the twisted policy is to keep code around as long as it isn't a maintenance burden. Applying this to documentation, there is no reason to remove documentation until the associated code is removed. Once the code is gone, it isn't useful, but it may potentially be, up to that point.
comment:26 Changed 7 years ago by
Replying to tom.prince:
Replying to hawkowl:
Replying to exarkun:
This is incorrect. The standing policy is to remove the documentation when the feature is removed. Not to remove the documentation when the feature is deprecated.
I didn't know this was part of policy, and I can't find it in the Documentation standard or Compatibility Policy -- should we add it, or is it somewhere else?
I'm not sure if it is written down anywhere (if it isn't it should be). The reasoning is, that until the code is gone, people may be using it and so we should keep existing documentation about it.
- Perhaps they inherited a codebase that is already using it, and they need to figure it out before removing the usage
- In a case like this, where we are removing functionality, they may have use of it, even if we aren't going to continue supporting it.
In general, the twisted policy is to keep code around as long as it isn't a maintenance burden. Applying this to documentation, there is no reason to remove documentation until the associated code is removed. Once the code is gone, it isn't useful, but it may potentially be, up to that point.
Right. This is an interesting case, because that example won't work (the endpoint is gone), and I'm not sure it could even work (does a 3rd party MSN server exist?). So, I'm not sure if it's really worth keeping, because it is essentially unhelpful.
comment:27 Changed 7 years ago by
It turns out MSN *isn't dead*. http://ismsndeadyet.com/ With the correct IP, the example can still work. So I'll do a branch that is pre-that.
comment:28 Changed 7 years ago by
Right. This is an interesting case, because that example won't work (the endpoint is gone), and I'm not sure it could even work (does a 3rd party MSN server exist?). So, I'm not sure if it's really worth keeping, because it is essentially unhelpful.
Or you could just do what I asked instead of trying to invent special cases and move on from this fairly irrelevant piece of work to more useful things.
Heck, if that had happened 22 months ago we could be *removing* the MSN implementation instead of still discussing how to deprecate it.
comment:29 Changed 7 years ago by
Branch: | branches/deprecate-msn-6395 → branches/deprecate-msn-6395-2 |
---|
(In [43684]) Branching to deprecate-msn-6395-2.
comment:30 Changed 7 years ago by
comment:31 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | hawkowl deleted |
Moved the topfile (didn't notice that last time), shifted the version numbers to 15.1, and forced a build on the buildbots. Since I modified it, I'll put it up for review.
comment:32 Changed 7 years ago by
I have fixed the pyflakes test and triggers a new round of tests.
Hope all is ok.
Otherwise the changes looks good.
Not sure if I have the right to merge this as I have also made a change... but the change is minor.
comment:33 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to hawkowl |
Thanks for the work on this.
All look good!
I have fixed the pyflakes error, but on the last buildbot run one test is failing on win xp... but I think that this is not related.
twisted.words.test.test_xpath.XPathTests.test_position =============================================================================== [FAIL] Traceback (most recent call last): File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\trial\_dist\test\test_disttrial.py", line 371, in test_runUntilFailure self.assertEqual(5, len(called)) File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\trial\_synctest.py", line 447, in assertEqual % (msg, pformat(first), pformat(second))) twisted.trial.unittest.FailTest: not equal: a = 5 b = 2
Please take a look and see if this is spurious or no... if is spurious maybe we should create a ticket and fix it. I am happy to work on that as I dislike having spurious tests.
comment:34 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Deleted twisted.words.protocols.msn