Concatenate strings instead of an instance #23

Merged
merged 2 commits into from May 24, 2016

Projects

None yet

2 participants

@timmuller
Contributor

When providing a filehander to the RarFile constructor, this.rarfile will not be a path (str), but an instance.
Due the error of concatenating an instance to a string when constructing a NonRarFile exception, an TypeError is raised instead of the expected NotRarFile exception.

Example stacktrace (py.test):

self = <rarfile.RarFile object at 0x7fbd02c9d810>

    def _parse_real(self):
        fd = XFile(self.rarfile)
        self._fd = fd
        id = fd.read(len(RAR_ID))
        if id != RAR_ID:
>           raise NotRarFile("Not a Rar archive: "+self.rarfile)
E           TypeError: cannot concatenate 'str' and 'instance' objects

../../.virtualenvs/byte-user-milter/local/lib/python2.7/site-packages/rarfile.py:789: TypeError

With this patch the NonRarFile message will contain the following:

NotRarFile: Not a Rar archive: <StringIO.StringIO instance at 0x7fa600b43098>
@timmuller timmuller Concatenate strings instead of an instance
When providing a filehander to the `RarFile` constructor, `this.rarfile` will not be a path (str), but an instance.
Due the error of concatenating an instance to a string when constructing a `NonRarFile` exception, an `TypeError` is raised instead of the expected `NotRarFile` exception.

Example stacktrace (py.test):
```
self = <rarfile.RarFile object at 0x7fbd02c9d810>

    def _parse_real(self):
        fd = XFile(self.rarfile)
        self._fd = fd
        id = fd.read(len(RAR_ID))
        if id != RAR_ID:
>           raise NotRarFile("Not a Rar archive: "+self.rarfile)
E           TypeError: cannot concatenate 'str' and 'instance' objects

../../.virtualenvs/byte-user-milter/local/lib/python2.7/site-packages/rarfile.py:789: TypeError

```
113375a
@markokr
Owner
markokr commented May 24, 2016

Good point.

But now that self.rarfile may be funny object, it seems much more straightforward to simply stop using it in exception strings. THB, from just looking at the diff, I have no idea what happens with "{}".format(file).

So for overall simplicity, the code seems better when you just remove the rarfile from message.

@timmuller timmuller Only append filepath to NonRarFile
Do not append instance-references to the NonRarFile message
f3d5d44
@timmuller
Contributor
timmuller commented May 24, 2016 edited

You're right. It's better not to log the instance-references within the exception, because it doesn't add extra information.
I changed to code based on your suggestion. Thanks for your quick reply!

@markokr markokr merged commit e3d566d into markokr:master May 24, 2016
@markokr
Owner
markokr commented May 24, 2016

isinstance check seems like overkill, but is probably more user-friendly. Merged.

Thanks!

@timmuller timmuller deleted the timmuller:concatenate_str branch May 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment