Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

assertRaises as a context manager should accept a 'msg' keyword argument. #54984

Closed
bitdancer opened this issue Dec 26, 2010 · 15 comments
Closed

assertRaises as a context manager should accept a 'msg' keyword argument. #54984

bitdancer opened this issue Dec 26, 2010 · 15 comments
Assignees
@ezio-melotti
Labels
easy type-feature A feature request or enhancement

Comments

@bitdancer
Copy link
Member

BPO 10775
Nosy @rhettinger, @ezio-melotti, @bitdancer, @voidspace, @briancurtin, @durban
Files
  • patch.diff: patch
  • issue10775-2.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ezio-melotti'
closed_at = <Date 2011-05-06.12:07:25.291>
created_at = <Date 2010-12-26.19:18:47.677>
labels = ['easy', 'type-feature']
title = "assertRaises as a context manager should accept a 'msg' keyword argument."
updated_at = <Date 2011-05-06.12:07:25.290>
user = 'https://github.com/bitdancer'

    bugs.python.org fields:

    activity = <Date 2011-05-06.12:07:25.290>
actor = 'ezio.melotti'
assignee = 'ezio.melotti'
closed = True
closed_date = <Date 2011-05-06.12:07:25.291>
closer = 'ezio.melotti'
components = []
creation = <Date 2010-12-26.19:18:47.677>
creator = 'r.david.murray'
dependencies = []
files = ['20765', '21839']
hgrepos = []
issue_num = 10775
keywords = ['patch', 'easy']
message_count = 15.0
messages = ['124675', '125169', '128623', '130840', '130897', '130907', '130943', '130966', '130968', '130970', '130988', '134849', '135276', '135278', '135279']
nosy_count = 10.0
nosy_names = ['rhettinger', 'ezio.melotti', 'r.david.murray', 'michael.foord', 'brian.curtin', 'SilentGhost', 'daniel.urban', 'Winston.Ewert', 'python-dev', 'robquad']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue10775'
versions = ['Python 3.3']
    @bitdancer
    Copy link
    Member Author

    assertRaises used as a method can't take a msg keyword argument because all args and keywords are passed to the callable. But in context manager form it could, and this can be useful. See, for example, bpo-3583.

    @bitdancer bitdancer added easy type-feature A feature request or enhancement labels Dec 26, 2010
    @voidspace
    Copy link
    Contributor

    I'm fine with this functionality being added in 3.3.

    @WinstonEwert
    Copy link
    Mannequin

    WinstonEwert mannequin commented Feb 16, 2011

    I decided to try my hand at writing a patch for python.

    I ended up implementing the behavior for assertRaises, assertRaisesRegex, assertWarns, and assertWarnsRegex. I also made those functions complain about other arguments rather then just ignoring them.

    @robquad
    Copy link
    Mannequin

    robquad mannequin commented Mar 14, 2011

    Changing callableObj to callable_obj in assertRaises will break for anyone that's upgrading to 3.3. I left a comment on the review.

    @ncoghlan
    Copy link
    Contributor

    Aren't such use cases already covered by assertRaisesRegex?

    @bitdancer
    Copy link
    Member Author

    How does assertRaisesRegex address the use case in bpo-3583?

    @WinstonEwert
    Copy link
    Mannequin

    WinstonEwert mannequin commented Mar 15, 2011

    robquad mentions having left a comment on the review, but I'm not seeing how to view it. Can somebody explain?

    It wasn't necessary to change the callable_obj bit, but both form were being used so I thought it best to standardize. Neither version of the parameter name shows up in the documentation.

    @ncoghlan
    Copy link
    Contributor

    Michael pointed out that I had completely missed the point of what the "msg" argument was about. Sorry for the noise.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Mar 15, 2011

    I left a comment on the review.
    You need to publish your comment if you want others to see it.

    @briancurtin
    Copy link
    Member

    I showed robquad how to do the review stuff at PyCon but I forgot about the publish part. Robbie, if you hit "Publish + Mail Comments" near the top of the page after you've left comments, it'll send them out.

    What he noticed was that changing to callable_obj in the assertRaises signature could break anyone who had been using callableObj as a named argument. Although it isn't explicitly documented, it's a named argument that someone is probably using. As for standardizing, it's probably best to match the general format of the library which is camelCase, and change the internal uses rather than a public method signature.

    @WinstonEwert
    Copy link
    Mannequin

    WinstonEwert mannequin commented Mar 15, 2011

    The public methods were using both callable_obj and callableObj. Perhaps the patch should standardize on callableObj and accept callable_obj with a warning?

    @voidspace voidspace self-assigned this Mar 15, 2011
    @ezio-melotti
    Copy link
    Member

    Attached a revised patch.
    While I agree that an error should be raised when extra args are provided in the context manager form, this is out of the scope of the issue, so I didn't include those changes.

    @voidspace
    Copy link
    Contributor

    New patch by Ezio looks good to me. Go ahead and commit. Please raise a separate issue for error reporting when invalid argument combinations are used. (i.e. additional keyword arguments but no callable.)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 6, 2011

    New changeset 8fc801ca9ea1 by Ezio Melotti in branch 'default':
    Issue bpo-10775: assertRaises, assertRaisesRegex, assertWarns, and assertWarnsRegex now accept a keyword argument 'msg' when used as context managers. Initial patch by Winston Ewert.
    http://hg.python.org/cpython/rev/8fc801ca9ea1

    @ezio-melotti
    Copy link
    Member

    Done, thanks for the patch!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @ezio-melotti ezio-melotti

    Labels
    easy type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @voidspace @briancurtin @bitdancer @ncoghlan @ezio-melotti