Opened 3 years ago

Closed 3 years ago

#5209 task closed fixed (fixed)

Deprecate twisted.web.google

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: web Keywords: easy
Cc: jknight, gregoire.leroy@…, thijs Branch: branches/deprecate-google-5209
(diff, github, buildbot, log)
Author: retenodus Launchpad Bug:

Description

The twisted.web.google module is a meager layer on top of the old HTTP client. It's untested, undocumented, and not particularly useful. Google also offers actual APIs for doing what it does.

The whole module should be deprecated.

Attachments (2)

5209.patch (1.8 KB) - added by retenodus 3 years ago.
Module deprecated
5209-2.patch (1.6 KB) - added by retenodus 3 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 3 years ago by retenodus

Module deprecated

comment:2 Changed 3 years ago by retenodus

  • Cc gregoire.leroy@… added
  • Keywords review added

comment:3 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to retenodus

Thanks!

  1. Please generate patches from the top of the source tree.
  2. I think the entire twisted.web.google module should have the deprecation, not the two names defined in it. See twisted/enterprise/util.py for an example. For testing, the import can be nested inside the test method to trigger the deprecation (and an import-time deprecation is just what we want users to see, so this makes sense). You may need to delete the module from sys.modules first to ensure it really gets imported (and to make the test work with trial --until-failure).
  3. Function arguments should have a space following the comma, eg Version('Twisted', 11, 1, 0)
  4. Test methods should always have a docstring explaining what is being tested.

Thanks again!

Changed 3 years ago by retenodus

comment:4 Changed 3 years ago by retenodus

  • Keywords review added

The new patch should be OK

comment:5 Changed 3 years ago by retenodus

  • Owner retenodus deleted

comment:6 Changed 3 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to retenodus

The example also needs to go if it's deprecated imo.

comment:7 Changed 3 years ago by exarkun

I don't particularly care in this case, but in general we do not remove documentation when we deprecate something. We remove documentation when we remove the thing.

comment:8 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/deprecate-google-5209

(In [32446]) Branching to 'deprecate-google-5209'

comment:9 Changed 3 years ago by exarkun

(In [32447]) Apply most of 5209-2.patch

refs #5209

comment:8 Changed 3 years ago by exarkun

  • Author exarkun deleted
  • Branch branches/deprecate-google-5209 deleted
  • Owner changed from retenodus to exarkun

I had some trouble applying this patch. Visual inspection doesn't reveal any obvious issues, but the patch program refused to apply the test_web.py and google.py hunks. Since it was a short patch, I applied it by hand. I suggest trying to figure out why it can't be applied automatically though, since this could be a bigger problem for bigger patches.

Some comments about the contents of the patch:

  1. The google.py changes look superfluous now, since the deprecation is done entirely in __init__.py. So I'll just drop them.
  2. A couple new lines have trailing whitespace, which we try to avoid, so I'll strip it from them.
  3. Deprecations should get a .removal news fragment (documented at ReviewProcess#Newsfiles).

Since these are simple, I'll make the adjustments and then merge. Thanks!

comment:9 Changed 3 years ago by exarkun

  • Author set to retenodus
  • Branch set to I had some trouble applying this patch. Visual inspection doesn't reveal any obvious issues, but the patch program refused to apply the `test_web.py` and `google.py` hunks. Since it was a short patch, I applied it by hand. I suggest trying to figure out why it can't be applied automatically though, since this could be a bigger problem for bigger patches. Some comments about the contents of the patch: 1. The `google.py` changes look superfluous now, since the deprecation is done entirely in `__init__.py`. So I'll just drop them. 1. A couple new lines have trailing whitespace, which we try to avoid, so I'll strip it from them. 1. Deprecations should get a `.removal` news fragment (documented at ReviewProcess#Newsfiles). Since these are simple, I'll make the adjustments and then merge. Thanks!

comment:10 Changed 3 years ago by exarkun

  • Branch changed from I had some trouble applying this patch. Visual inspection doesn't reveal any obvious issues, but the patch program refused to apply the `test_web.py` and `google.py` hunks. Since it was a short patch, I applied it by hand. I suggest trying to figure out why it can't be applied automatically though, since this could be a bigger problem for bigger patches. Some comments about the contents of the patch: 1. The `google.py` changes look superfluous now, since the deprecation is done entirely in `__init__.py`. So I'll just drop them. 1. A couple new lines have trailing whitespace, which we try to avoid, so I'll strip it from them. 1. Deprecations should get a `.removal` news fragment (documented at ReviewProcess#Newsfiles). Since these are simple, I'll make the adjustments and then merge. Thanks! to branches/deprecate-google-5209

Jesus H, what a failure.

comment:13 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [32449]) Merge deprecate-google-5209

Author: retenodus
Reviewer: thijs, exarkun
Fixes: #5209

Deprecate all of twisted.web.google.

Note: See TracTickets for help on using tickets.