python / cpython

The Python programming language
https://www.python.org
Other
61.98k stars 29.8k forks source link

typos in curses argument error messages #60056

Closed cjerdonek closed 6 years ago

cjerdonek commented 11 years ago
BPO 15852
Nosy @ezio-melotti, @bitdancer, @cjerdonek, @berkerpeksag, @serhiy-storchaka, @phmc
PRs
  • python/cpython#4950
  • python/cpython#4952
  • python/cpython#4951
  • Files
  • issue-15852-1.patch
  • 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: ```python assignee = None closed_at = created_at = labels = ['3.7', 'easy', 'type-bug', 'library'] title = 'typos in curses argument error messages' updated_at = user = 'https://github.com/cjerdonek' ``` bugs.python.org fields: ```python activity = actor = 'serhiy.storchaka' assignee = 'none' closed = True closed_date = closer = 'asvetlov' components = ['Library (Lib)'] creation = creator = 'chris.jerdonek' dependencies = [] files = ['27106'] hgrepos = [] issue_num = 15852 keywords = ['patch', 'easy', 'needs review'] message_count = 12.0 messages = ['169725', '169727', '169729', '169730', '169736', '169737', '170871', '170900', '170927', '187287', '305388', '308857'] nosy_count = 6.0 nosy_names = ['ezio.melotti', 'r.david.murray', 'chris.jerdonek', 'berker.peksag', 'serhiy.storchaka', 'pconnell'] pr_nums = ['4950', '4952', '4951'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue15852' versions = ['Python 2.7', 'Python 3.6', 'Python 3.7'] ```

    cjerdonek commented 11 years ago

    It seems like a couple error messages in the curses module need correcting:

    PyErr_SetString(PyExc_TypeError, "insch requires 1 or 4 arguments");

    http://hg.python.org/cpython/file/8ff2f4634ed8/Modules/_cursesmodule.c#l1322

    PyErr_SetString(PyExc_TypeError, "instr requires 0 or 3 arguments");

    http://hg.python.org/cpython/file/8ff2f4634ed8/Modules/_cursesmodule.c#l1385

    In both cases, "or" should be "to".

    bitdancer commented 11 years ago

    Given their signatures in the docs, I suspect it is more complicated than that. Perhaps the error messages are even correct. What does the code implement?

    cjerdonek commented 11 years ago

    I just completed a patch to improve the documentation of these signatures (along with many others) in bpo-15831. The correction here is consistent with my findings and revised documentation there.

    As for the code, they are straightforward switch statements similar to many of the other methods in that module. I also did a manual test on one of the methods as a sanity check. I am preparing unit tests.

    cjerdonek commented 11 years ago

    The situation is the same in 2.7 (and probably 3.2).

    cjerdonek commented 11 years ago

    Attaching a patch with tests and fix for the default branch.

    cjerdonek commented 11 years ago

    Uploading correct file.

    ezio-melotti commented 11 years ago

    Can't you use assertRaisesRegex?

    cjerdonek commented 11 years ago

    Thanks for taking a look at this, and good question.

    Without restructuring how the tests are done, I believe the short answer is no. The funny thing about this test module is that it does not actually have any unittest test cases. It just calls some functions. Failure happens if an exception is raised in any one of those functions.

    See here, for example:

    http://hg.python.org/cpython/file/59a2807872d5/Lib/test/test_curses.py#l35

    cjerdonek commented 11 years ago

    FYI, I created bpo-16000 :) to switch test_curses to using unittest.TestCase.

    0f33552e-3426-4ce7-83ad-4a88a6ffc812 commented 11 years ago

    The patch looks correct and complete, and still patches and passes the tests.

    So, as far as I can see, this can be committed.

    serhiy-storchaka commented 6 years ago

    Do you mind to create a pull request on GitHub Chris?

    In general the patch LGTM, but I don't think this minor typo fix needs tests for exact error messages. Adding new tests is good, but I think it is enough to test that corresponding functions accept the correct number of arguments and raise TypeError on incorrect number of arguments.

    serhiy-storchaka commented 6 years ago

    It was worth to honor Chris as the author of the patch in the commit message.