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.

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

bpo-34193: Fix pluralization in getargs.c and test cases #8438

Merged
merged 4 commits into from Dec 21, 2018

Conversation

tirkarthi
Copy link
Member

@tirkarthi tirkarthi commented Jul 24, 2018

Tests: In the 3 tests before this, the suffix 0, 2, or 3 is the number of arguments given. This would suggest 0a, 0b, 2a, and 3a for the suffixes here. I like 1min, 2min, 1max, and 2max even better. You can wait a bit for other reviews before changing.

I have changed the function names but I can change as per review comments

But if we add all the remaining fixes needed in this file (see below), we could write.
Fix pluralization in TypeError messages in getargs.c: '1 argument' instead of '1 arguments' and '1 element' instead of '1 elements'.

Added a new blurb. Let me know if the old one needs edit and can be re-used or to have the current one.

Scope: I think we should finish the job in the file. The next line in both else clauses,
"unpacked tuple should have %s%zd elements,"
should get the same treatment. This may break existing tests. (These are the only two formatted 'elements'.
Fixes for the following will break some test, which will then need fixing also.
542: "expected %d arguments, not %" PY_FORMAT_SIZE_T "d",
1720, "%.200s%s takes %s %d positional arguments"
1799, "%.200s%s takes %s %d positional arguments"
2106, "%200s%s takes %s %d positional arguments (%d given)",
2154: "%.200s%s takes %s %d positional arguments"

I have fixed the above ones and there were some test case failures in test_capi and test_getargs which were fixed. Changes for 1799 line caused test case failures that were fixed.

With respect to elements the code path is triggered only when fname is set as NULL and I couldn't find a function that passes fname as NULL. Almost all of them pass it as "" in case it's not present. I commented out the else block and tried running the test suite and had no failures. I think the code is not hit by any function. They were added as part of e4616e6 in 2001 and there are no test cases.

I tried looking into argument clinic docs to see if I can come up with a format string and function to hit the respective code paths for the patch. But understanding the format string is little difficult for me since the ones I can come up with generate error with required argument and not positional argument. I think test coverage could be improved in these and I also don't know if we have tests that hit this code but don't check for the error message. I will try my hands at _testcapimodule.c to see if I can come up with tests. Any guidance on testing and argument clinic format string will be highly helpful.

Thanks

https://bugs.python.org/issue34193

@tirkarthi
Copy link
Member Author

tirkarthi commented Dec 21, 2018

Thanks @serhiy-storchaka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants