bpo-34248: add filename to DbmError at gdbm_open()#8590
bpo-34248: add filename to DbmError at gdbm_open()#8590berkerpeksag merged 1 commit intopython:masterfrom
Conversation
csernazs
force-pushed
the
fix-issue-34248
branch
2 times, most recently
from
b00ee98 to
a7e6a2a
Compare
There was a problem hiding this comment.
You should use filename instead of TESTFN for consistency with the other tests in this file.
This self.assertFalse() should probably be replaced by a simple unlink(filename) (like in test_error_conditions()).
There was a problem hiding this comment.
You should probably use assertRaisesRegex() to check the exception message (instead of the separate assertEqual() call).
There was a problem hiding this comment.
I don't think this test is needed. It's also missing a unittest.skipUnless() decorator for TESTFN_NONASCII.
There was a problem hiding this comment.
I think you meant to use TESTFN_NONASCII here?
csernazs
force-pushed
the
fix-issue-34248
branch
2 times, most recently
from
6624138 to
4a74533
Compare
There was a problem hiding this comment.
assertRaisesRegexp() is deprecated. Please use assertRaisesRegex().
There was a problem hiding this comment.
Why do we need this? Can't we just call gdbm.open() with an non-existing file name?
There was a problem hiding this comment.
Which filename should be there which is guaranteed to be non-existing? I think we should use a file which is under our control and thereby I re-used the filename variable.
The unlink is a very last resort to ensure that the file won't exist, this would normall not remove any file as it is already removed in tearDown.
There was a problem hiding this comment.
Using something like gdbm.open('nonexisting-file') is enough.
There was a problem hiding this comment.
It is better to use raw string literals for regexes.
Since filename can contain regex metacharacters, it should be wrapped with re.escape() if used in a regex pattern.
Errno 2 is platform depending.
Is the filename attribute of the exception set?
with self.assertRaises(gdbm.error) as cm:
gdbm.open(filename)
self.assertIn(filename, str(cm.exception))
self.assertEqual(cm.exception.filename, filename) # is this true?
csernazs
force-pushed
the
fix-issue-34248
branch
2 times, most recently
from
3ff85c6 to
4a7cad7
Compare
|
I've added ndbm also to this commit due to the request in the bpo ticket. If you want to have this in two commits, let me know. Thanks! |
…dbm.open Report the filename to the exception when raising error in dbm.gnu.open and db.ndbm.open functions, so it gets printed when the exception is unhandled, and also can be obtained by the filename attribute of the exception object.
csernazs
force-pushed
the
fix-issue-34248
branch
from
96e9155 to
6035eb6
Compare
|
@berkerpeksag could you please help how to have this pull request merged to master? Is there any action required from my side? I think I have made everything which was requested. |
|
I had to close and reopen thid because the AppVeyor build wasn't completed. |
|
Thanks! |
|
Thank you! |
|
@berkerpeksag should we backport of this patch to 3.7? It seems for me that this patch was not included in 3.7.1. Should I need to open a new PR for the backport? |
|
New features can only go into the next feature release, which will be 3.8. 3.7 is now in maintenance mode, so we can't backport it into 3.7. |
6 participants
Add the filename to the DbmError raised when dbm.gnu.open() is called and
errno is set.
https://bugs.python.org/issue34248