[Django] #29658: Use unittest.addCleanup to unregister lookups.

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
#29658: Use unittest.addCleanup to unregister lookups.
------------------------------------------------+------------------------
               Reporter:  Mads Jensen           |          Owner:  nobody
                   Type:  Cleanup/optimization  |         Status:  new
              Component:  Core (Other)          |        Version:  master
               Severity:  Normal                |       Keywords:
           Triage Stage:  Unreviewed            |      Has patch:  0
    Needs documentation:  0                     |    Needs tests:  0
Patch needs improvement:  0                     |  Easy pickings:  1
                  UI/UX:  0                     |
------------------------------------------------+------------------------
 There are some places in the test suite that follow this pattern:

 {{{
 try:
    CharField.register_lookup(Length)
    # some test code
 finally:
    CharField._unregister_lookup(Length)
 }}}

 It'd be better to use `unittest.addCleanup` to handle this stuff.

--
Ticket URL: <https://code.djangoproject.com/ticket/29658>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/053.9be36496ddb2273263fbed07ed805159%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
#29658: Use unittest.addCleanup to unregister lookups.
--------------------------------------+------------------------------------
     Reporter:  Mads Jensen           |                    Owner:  nobody
         Type:  Cleanup/optimization  |                   Status:  new
    Component:  Core (Other)          |                  Version:  master
     Severity:  Normal                |               Resolution:
     Keywords:                        |             Triage Stage:  Accepted
    Has patch:  0                     |      Needs documentation:  0
  Needs tests:  0                     |  Patch needs improvement:  0
Easy pickings:  1                     |                    UI/UX:  0
--------------------------------------+------------------------------------
Changes (by Claude Paroz):

 * stage:  Unreviewed => Accepted


--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.024f7617bb8eae6438870ecad5ae1a03%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
In reply to this post by Django
#29658: Use unittest.addCleanup to unregister lookups.
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Dominic
         Type:                       |  White
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Dominic White):

 * status:  new => assigned
 * owner:  nobody => Dominic White


--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.9925e9f2ef8595da71a88cdbc1f9996a%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
In reply to this post by Django
#29658: Use unittest.addCleanup to unregister lookups.
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Dominic
         Type:                       |  White
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Srinivas Reddy Thatiparthy):

 https://code.djangoproject.com/ticket/29658

--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.a4fca27c66ecd246a9d9a0737d0724da%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
In reply to this post by Django
#29658: Use unittest.addCleanup to unregister lookups.
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Srinivas
         Type:                       |  Reddy Thatiparthy
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Srinivas Reddy Thatiparthy):

 * owner:  Dominic White => Srinivas Reddy Thatiparthy


--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.6999b9600c0db2bca0cfe51e759a870d%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
In reply to this post by Django
#29658: Use unittest.addCleanup to unregister lookups.
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Srinivas
         Type:                       |  Reddy Thatiparthy
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Claude Paroz):

 * has_patch:  0 => 1


Comment:

 The current patch is using a context manager instead of the `try/finally`
 method.
 Mads, is that something you are comfortable with, or do you still see some
 benefit with `addCleanup`?

--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.39c70c098b7573ee419bda488246e99b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
In reply to this post by Django
#29658: Use unittest.addCleanup to unregister lookups.
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Srinivas
         Type:                       |  Reddy Thatiparthy
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Ready for
                                     |  checkin
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):

 * stage:  Accepted => Ready for checkin


Comment:

 Patch looks good to me, unless there's some reason to prefer
 `addCleanup()`.

--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.5708d780cd2588bf8244d62c3e92864e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use unittest.addCleanup to unregister lookups.

Django
In reply to this post by Django
#29658: Use unittest.addCleanup to unregister lookups.
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Srinivas
         Type:                       |  Reddy Thatiparthy
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Ready for
                                     |  checkin
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

 Using context managers should be fine as well.

--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.c19684c6813b9f73f97490a3a2e41723%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use a context manager to unregister model field lookups in tests (was: Use unittest.addCleanup to unregister lookups.)

Django
In reply to this post by Django
#29658: Use a context manager to unregister model field lookups in tests
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Srinivas
         Type:                       |  Reddy Thatiparthy
  Cleanup/optimization               |                   Status:  assigned
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Ready for
                                     |  checkin
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Description changed by Tim Graham:

Old description:

> There are some places in the test suite that follow this pattern:
>
> {{{
> try:
>    CharField.register_lookup(Length)
>    # some test code
> finally:
>    CharField._unregister_lookup(Length)
> }}}
>
> It'd be better to use `unittest.addCleanup` to handle this stuff.
New description:

 There are some places in the test suite that follow this pattern:

 {{{
 try:
    CharField.register_lookup(Length)
    # some test code
 finally:
    CharField._unregister_lookup(Length)
 }}}

 It'd be better to use a context manager (`addCleanup()` was originally
 proposed) to handle this stuff.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.8c779130db422703616c1f0488625355%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29658: Use a context manager to unregister model field lookups in tests

Django
In reply to this post by Django
#29658: Use a context manager to unregister model field lookups in tests
-------------------------------------+-------------------------------------
     Reporter:  Mads Jensen          |                    Owner:  Srinivas
         Type:                       |  Reddy Thatiparthy
  Cleanup/optimization               |                   Status:  closed
    Component:  Core (Other)         |                  Version:  master
     Severity:  Normal               |               Resolution:  fixed
     Keywords:                       |             Triage Stage:  Ready for
                                     |  checkin
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham <timograham@…>):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"233c70f0479beb3bff9027e6cff680882978fd4d" 233c70f0]:
 {{{
 #!CommitTicketReference repository=""
 revision="233c70f0479beb3bff9027e6cff680882978fd4d"
 Fixed #29658 -- Registered model lookups in tests with a context manager.
 }}}

--
Ticket URL: <https://code.djangoproject.com/ticket/29658#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/068.65a18aca9bb0d99128dc5af4a46c3ace%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.