Deprecate twisted.words.protocols.msn

[therve]

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.

[Indradhanush Gupta]

Deleted twisted.words.protocols.msn

[Thijs Triemstra]

Hi your patch seems to be incorrect. Why is this breaking the compatibility policy by not deprecating it first?

[Indradhanush Gupta]

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.

[Indradhanush Gupta]

Deprecated twisted.words.protocols.msn

[Itamar Turner-Trauring]

Thanks for the patch!

1. The comment is unnecessary, it's pretty clear what's going on.
2. Please run diff at the root of the Twisted checkout, so it's clear which file the patch applies to.
3. All code changes to Twisted need tests. twisted/conch/test/test_insults.py should provide an example of how to test deprecation of modules.
4. The patch should include a news file (https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles).

Please address these points and resubmit your patch.

[Indradhanush Gupta]

Added NEWS File and tests to deprecate twisted.words.protocols.msn

[Indradhanush Gupta]

Applied the corrections.

[Tom Prince]

1. Did you try running the tests? The code fails to run for me. (But see below)
2. 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.

3. Calls to flushWarnings should pass the function that is to be blamed by the warning (in this case test_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)

[Indradhanush Gupta]

Made changes according to review points. But trial twisted generates 2 errors.

[Indradhanush Gupta]

Made changes according to review points. But trial twisted generates 2 errors.

[Indradhanush Gupta]

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.

[Indradhanush Gupta]

I am interested in participating in GSoC 13 under Twisted. itamar suggested to assign it to you.

[Tom Prince]

1. 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).
2. 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 the len(warnings) is 0 not 1, rather then having to realise that IndexError means that warnings isnt long enough.

[moijes12]

Completing dhanush's work. Tests have been run against the patch and they pass. Also, the warning is now raised.

[Thijs Triemstra]

Thanks for your patch.

1. the version nr should be bumped to 14.1
2. there's still an example in [source:trunk/docs/words/examples/msn_example.py] that needs to be removed

[moijes12]

Building on thijs' comments. Attaching a tested patch.

[hawkowl]

(In [43363]) Branching to deprecate-msn-6395.

[hawkowl]

(In [43364]) applying patch from moijes12, refs #6395

[hawkowl]

LGTM. I'll merge it tomorrow.

[Jean-Paul Calderone]

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.

[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?

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

[hawkowl]

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.

[hawkowl]

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.

[Jean-Paul Calderone]

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.

[hawkowl]

(In [43684]) Branching to deprecate-msn-6395-2.

[hawkowl]

(In [43685]) applying patch from moijes12, refs #6395 + moving it to 15.1

[hawkowl]

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.

[Adi Roiban]

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.

[Adi Roiban]

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.

[hawkowl]

(In [43763]) Merge deprecate-msn-6395-2: Deprecate MSN support

Authors: dhanush, moijes12, adiroiban Reviewers: itamar, tom.prince, thijs, hawkowl, adiroiban Fixes: #6395