Summary

ExtensionInfo can not parse PKG-INFO with multiline description

Review Request #8001 — Created Feb. 26, 2016 and submitted March 11, 2016, 12:53 p.m.

Information

Owner

iconoeugen

Repository

Djblets

Branch

master

Bugs

4196

Depends On

Reviewers

Groups

djblets

People

Description

ExtensionInfo can not parse PKG-INFO with multiline description

Testing Done

Testing done also with ReviewBoard

Issues

Description	From	Last Updated
Col: 11 E111 indentation is not a multiple of four	reviewbot	Feb. 26, 2016, 8:27 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Feb. 26, 2016, 8:27 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Feb. 26, 2016, 8:27 a.m.
Alphabetize	brennie	March 1, 2016, 12:24 p.m.
Single quotes on 'PKG-INFO'	brennie	Feb. 29, 2016, 2:38 a.m.
blank line between these.	brennie	Feb. 29, 2016, 2:39 a.m.
Blank line between these.	brennie	Feb. 29, 2016, 2:39 a.m.
This does not handle the case where we cannot decode using any of the encodings.	brennie	March 1, 2016, 4:20 a.m.
Blank line between these.	brennie	Feb. 29, 2016, 2:39 a.m.
If data is empty, we don't really need to go through the above, do we?	brennie	March 1, 2016, 4:19 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 1, 2016, 12:35 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 1, 2016, 12:35 p.m.
This really should be a constant on the class.	brennie	March 3, 2016, 5:49 a.m.
Can you reflow this? Each sentence needn't take up a whole line.	brennie	March 3, 2016, 5:49 a.m.
Blank line between these.	brennie	March 3, 2016, 5:49 a.m.
Single quotes. Can you move the % to the next line?	brennie	March 3, 2016, 5:49 a.m.
Single quotes. Can you move the % to the next line?	brennie	March 3, 2016, 5:49 a.m.
Please add a period at the end.	david	March 8, 2016, 1:55 a.m.
It would be a little nicer to write this using for..else (avoiding the "decoded" variable): for enc in self.encodings: try: …	david	March 8, 2016, 1:55 a.m.

Status: Discarded

Status: Re-opened

Testing Done:

Testing done also with ReviewBoard

Bugs:

+	4196

Diff:

Revision 1 (+15 -7)

djblets/extensions/extension.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py

djblets/extensions/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```
djblets/extensions/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```
djblets/extensions/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```

Diff:

Revision 2 (+14 -6)

Show changes

djblets/extensions/extension.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py

djblets/extensions/extension.py (Diff revision 2)

The issue has been resolved. Show all issues

Alphabetize

iconoeugen Feb. 29, 2016, 5:16 a.m.

Can you be more explicit? What do you really mean by this?

brennie Feb. 29, 2016, 11:04 a.m.

Sorry, I mean that your imports should be in alphabetical order, e.g.:

import locale
import os

djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Single quotes on 'PKG-INFO'
```
djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
blank line between these.
```
djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

djblets/extensions/extension.py (Diff revision 2)

The issue has been resolved. Show all issues

This does not handle the case where we cannot decode using any of the encodings.

iconoeugen Feb. 29, 2016, 5:16 a.m.

In case that none of the decode methods are working the data will remain unchanged (joined multiline string) with the "hope" that feed method will be able to cope with that.

This would be a good candidate for logging, but I haven't seen any use of it in this file and didn't wanted to introduce it ;)

The other option would be to throw an exception, but djblets/reviewboard implementation has a problem with it and it's craching the server.

What would be your prefered way forward?

brennie Feb. 29, 2016, 11:04 a.m.

Feel free to add some logging, probably at INFO or WARNING levels.

I don't think throwing an exception here would be best. We should probably document that if decoding did not succeed, the method will continue as if it did.

djblets/extensions/extension.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

djblets/extensions/extension.py (Diff revision 2)

The issue has been resolved. Show all issues

If data is empty, we don't really need to go through the above, do we?

iconoeugen Feb. 29, 2016, 5:16 a.m.

You are right. The data is initialized by the join and if the returned iterator is empty the data will end up with the value '', so it can never be None.
As far as it goes to skiping/doing the call of the feed method if the date is empty, it must be opted between the two cases:

always do the feed call which will keep the code cleaner and the feed method will act accordingly
skip the feed call to gain on performance side, but increase the cyclomatic compexity of the method.

For me it's no issue to introduce the condition statement, just let me know what would you like to have.

brennie Feb. 29, 2016, 11:04 a.m.

We should probably go with cleaner, simpler code.

Change Summary:

Introduced fixes for comments and some feedback for still open points

Diff:

Revision 3 (+15 -5)

Show changes

djblets/extensions/extension.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py



Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py

Change Summary:

Added logging for extension info decode

Diff:

Revision 4 (+28 -5)

Show changes

djblets/extensions/extension.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py

djblets/extensions/extension.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
djblets/extensions/extension.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```

Change Summary:

Fixed pep8 E501 line too long

Diff:

Revision 5 (+30 -5)

Show changes

djblets/extensions/extension.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py

djblets/extensions/extension.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This really should be a constant on the class.
```
djblets/extensions/extension.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Can you reflow this? Each sentence needn't take up a 

whole line.
```
djblets/extensions/extension.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Blank line between these.
```
djblets/extensions/extension.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes. Can you move the % to the next line?
```
djblets/extensions/extension.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes. Can you move the % to the next line?
```

Change Summary:

Made encodings as ExtensionInfo class constant

Small style fixes

Diff:

Revision 6 (+31 -5)

Show changes

djblets/extensions/extension.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py

djblets/extensions/extension.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Please add a period at the end.
```

djblets/extensions/extension.py (Diff revision 6)

The issue has been resolved. Show all issues

It would be a little nicer to write this using for..else (avoiding the "decoded" variable):

for enc in self.encodings:
    try:
        data = data.decode(enc)
        break
    except UnicodeDecodeError:
        continue
else:
    logging.warning(
        'Failed to decode PKG-INFO content for extension %s',
        entrypoint.name)

We also don't need to log in the success case.

iconoeugen March 8, 2016, 2:08 a.m.

Thanks for the hint, nice to know one more Python specific

Change Summary:

Reimplemented decode check with for: else: syntax

Diff:

Revision 7 (+24 -6)

Show changes

djblets/extensions/extension.py

Tool: Pyflakes
Processed Files:
    djblets/extensions/extension.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/extensions/extension.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.8.x (3fc013a)