Opened 4 years ago

Closed 2 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: ralphm, 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)

deleted_msn.patch (27 bytes) - added by Indradhanush Gupta 4 years ago.
Deleted twisted.words.protocols.msn
deprecated_msn.patch (430 bytes) - added by Indradhanush Gupta 4 years ago.
Deprecated twisted.words.protocols.msn
deprecated_msn_added_corrections.patch (2.3 KB) - added by Indradhanush Gupta 4 years ago.
Added NEWS File and tests to deprecate twisted.words.protocols.msn
deprecated_msn_added_corrections_2.patch (2.6 KB) - added by Indradhanush Gupta 4 years ago.
Made changes according to review points. But trial twisted generates 2 errors.
deprecated_msn_added_corrections_2.2.patch (2.6 KB) - added by Indradhanush Gupta 4 years ago.
Made changes according to review points. But trial twisted generates 2 errors.
6395.patch (2.1 KB) - added by moijes12 3 years ago.
Completing dhanush's work. Tests have been run against the patch and they pass. Also, the warning is now raised.
6395-2.patch (4.4 KB) - added by moijes12 3 years ago.
Building on thijs' comments. Attaching a tested patch.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: ralphm added

Changed 4 years ago by Indradhanush Gupta

Attachment: deleted_msn.patch added

Deleted twisted.words.protocols.msn

comment:2 Changed 4 years ago by Indradhanush Gupta

Keywords: review added

comment:3 Changed 4 years ago by Thijs Triemstra

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 in reply to:  3 Changed 4 years ago by Indradhanush Gupta

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 4 years ago by Indradhanush Gupta

Attachment: deprecated_msn.patch added

Deprecated twisted.words.protocols.msn

comment:5 Changed 4 years ago by Indradhanush Gupta

Keywords: review added

comment:6 Changed 4 years ago by Indradhanush Gupta

Keywords: review removed

comment:7 Changed 4 years ago by Indradhanush Gupta

Keywords: review added

comment:8 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Indradhanush Gupta

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.

Changed 4 years ago by Indradhanush Gupta

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

comment:9 Changed 4 years ago by Indradhanush Gupta

Keywords: review added

Applied the corrections.

comment:10 Changed 4 years ago by Indradhanush Gupta

Owner: Indradhanush Gupta deleted

comment:11 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Indradhanush Gupta
  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.

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

Changed 4 years ago by Indradhanush Gupta

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

Changed 4 years ago by Indradhanush Gupta

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

comment:12 Changed 4 years ago by Indradhanush Gupta

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 4 years ago by Indradhanush Gupta

Owner: Indradhanush Gupta deleted

comment:14 Changed 4 years ago by Indradhanush Gupta

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 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Indradhanush Gupta
  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.

Changed 3 years ago by moijes12

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 3 years ago by moijes12

Keywords: review added
Owner: Indradhanush Gupta deleted

comment:17 Changed 3 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to moijes12
Summary: Delete twisted.words.protocols.msnDeprecate twisted.words.protocols.msn

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

Changed 3 years ago by moijes12

Attachment: 6395-2.patch added

Building on thijs' comments. Attaching a tested patch.

comment:18 Changed 3 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted

comment:19 Changed 2 years ago by hawkowl

Owner: set to hawkowl

comment:20 Changed 2 years ago by hawkowl

Author: hawkowl
Branch: branches/deprecate-msn-6395

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

comment:21 Changed 2 years ago by hawkowl

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

comment:22 Changed 2 years ago by hawkowl

Keywords: review removed

LGTM. I'll merge it tomorrow.

comment:23 Changed 2 years ago by 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.

comment:24 in reply to:  23 ; Changed 2 years ago by 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?

comment:25 in reply to:  24 ; Changed 2 years ago by 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.

comment:26 in reply to:  25 Changed 2 years ago by 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.

comment:27 Changed 2 years ago by 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.

comment:28 Changed 2 years ago by 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.

comment:29 Changed 2 years ago by hawkowl

Branch: branches/deprecate-msn-6395branches/deprecate-msn-6395-2

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

comment:30 Changed 2 years ago by hawkowl

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

comment:31 Changed 2 years ago by hawkowl

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 2 years ago by 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.

comment:33 Changed 2 years ago by Adi Roiban

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 2 years ago by hawkowl

Resolution: fixed
Status: newclosed

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

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

Note: See TracTickets for help on using tickets.