◐ Shell
reader mode source ↗
Skip to content

bpo-15988: make various overflow messages more consistent and helpful#668

Closed
orenmn wants to merge 21 commits into
python:masterfrom
orenmn:bpo15988-make-overflow-msgs-more-consistent
Closed

bpo-15988: make various overflow messages more consistent and helpful#668
orenmn wants to merge 21 commits into
python:masterfrom
orenmn:bpo15988-make-overflow-msgs-more-consistent

Conversation

@orenmn

@orenmn orenmn commented Mar 14, 2017

Copy link
Copy Markdown
Contributor

according to http://bugs.python.org/issue15988:

  1. in Python/getargs.c, patch seterror so that it would be easier to do 2 in various places that use PyArg_* functions.
  2. make various OverflowError and ValueError messages more consistent and helpful
  3. add various overflow tests

(I ran the test module, and on my Windows 10, the same tests failed with
and without my patches. However, on my Ubuntu 16.04 VM, none of the tests
failed.)

orenmn added 21 commits March 10, 2017 23:32
@orenmn
init commit
f9a126f
@orenmn
remove a redundant test of timedelta's constructor, and remove tests … …
37fc195 
…i added to csv, as the inconsistent message in csv was already fixed, and so my patch isn't related to it anymore
@orenmn
added a comment and removed a redundant test
c35fc35
@orenmn
solve conflict in Modules/io/bytesio.c while merging master
b6cc2ea
@orenmn
merged master
bba2087
@orenmn
remove functions names from error messages, according to the conventi… …
fbb56ab 
…on of the file
@orenmn
fixed an lzma test, removed my captain obvious comments from test_mem… …
8091b62 
…oryio, and optimized an if in seterror
@orenmn
in test_mmap: removed redundant test, and improved comments
f36f579
@orenmn
Unix, not unix
343b679
@orenmn
remove some more of my captain obvious comments
bb97755
@orenmn
in test_posix: move test inside @cpython_only, and undo removing an o… …
34e34ab 
…riginal test
@orenmn
remove from socketmodule.c a port boundaries check I have added, and … …
a88ecc5 
…fix my socket tests
@orenmn
add comment to test_stat
07387ec
@orenmn
fixed error message in blake
458dbc7
@orenmn
fixed array error messages
dca2943
@orenmn
fixed some error messages in selectmodule.c, and fixed argument to Py… …
f752b12 
…Arg_* funcs in mmapmodule.c
@orenmn
fixed some error messages in bytearrayobject.c and bytesobject.c, and… …
043ea7e 
… remove redundant code in _PyLong_AsInt
@orenmn
fix _PyLong_AsInt and remove a not-relevant-anymore comment from form… …
499ffc3 
…at_long_internal
@orenmn
remove redundant new line
82f3125
@orenmn
fixed a test in test_long
f0b59b7
@orenmn
added a skipUnless in test_posix
abb2977
@serhiy-storchaka serhiy-storchaka self-requested a review March 15, 2017 05:27
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Mar 15, 2017
serhiy-storchaka
serhiy-storchaka requested changes Mar 15, 2017
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

This is a great patch! Thanks Oren.

I made the first turn of reviewing. In general LGTM, but I left a number of comments. I think some changes could be extracted to separate issues. Some OverflowErrors could be avoided at all. I didn't tested all changes. I am not sure that reraising OverflowError with new error messages is appropriate in all cases. In some cases current error message looks not obviously worse.

23 hidden conversations Load more…
@serhiy-storchaka

serhiy-storchaka commented Sep 24, 2017

Copy link
Copy Markdown
Member

@orenmn, could you please merge with master and resolve conflicts? This will help further reviewing this giant PR.

Some issues may be already solved, perhaps in slightly different way.

@orenmn

orenmn commented Sep 25, 2017

Copy link
Copy Markdown
Contributor Author

I still have some small issues that i wish to take care of before diving into this one, but i hope to get to it sometime soon.

@orenmn

orenmn commented Sep 28, 2017

Copy link
Copy Markdown
Contributor Author

This PR is currently irrelevant, as explained in https://bugs.python.org/issue15988#msg303192

49 hidden items Load more…
@brettcannon

brettcannon commented Feb 21, 2018

Copy link
Copy Markdown
Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

@csabella

csabella commented May 29, 2019

Copy link
Copy Markdown
Contributor

Based on @orenmn's last message and the merge conflicts, I'm going to close this pull request. I believe @serhiy-storchaka was suggesting for this to be split into smaller pull requests that would be easier to review. If that's not the case, feel free to re-open this PR.

@csabella csabella closed this May 29, 2019
@serhiy-storchaka serhiy-storchaka mentioned this pull request Dec 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka requested changes

Assignees

No one assigned

Labels

awaiting changes type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@orenmn @serhiy-storchaka @brettcannon @csabella @Mariatta @the-knights-who-say-ni