This change has been marked as completed.

Describe the completed change (optional):

Pushed to release-0.8.x (3fc013a)

Summary

ExtensionInfo can not parse PKG-INFO with multiline description

Review Request #8001 — Created Feb. 26, 2016 and submitted 9 years, 1 month ago

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	9 years, 1 month ago
Col: 11 E111 indentation is not a multiple of four	reviewbot	9 years, 1 month ago
Col: 11 E111 indentation is not a multiple of four	reviewbot	9 years, 1 month ago
Alphabetize	brennie	9 years, 1 month ago
Single quotes on 'PKG-INFO'	brennie	9 years, 1 month ago
blank line between these.	brennie	9 years, 1 month ago
Blank line between these.	brennie	9 years, 1 month ago
This does not handle the case where we cannot decode using any of the encodings.	brennie	9 years, 1 month ago
Blank line between these.	brennie	9 years, 1 month ago
If data is empty, we don't really need to go through the above, do we?	brennie	9 years, 1 month ago
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	9 years, 1 month ago
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	9 years, 1 month ago
This really should be a constant on the class.	brennie	9 years, 1 month ago
Can you reflow this? Each sentence needn't take up a whole line.	brennie	9 years, 1 month ago
Blank line between these.	brennie	9 years, 1 month ago
Single quotes. Can you move the % to the next line?	brennie	9 years, 1 month ago
Single quotes. Can you move the % to the next line?	brennie	9 years, 1 month ago
Please add a period at the end.	david	9 years, 1 month ago
It would be a little nicer to write this using for..else (avoiding the "decoded" variable): for enc in self.encodings: try: …	david	9 years, 1 month ago
There are no open issues

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 9 years, 1 month ago

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

brennie 9 years, 1 month ago

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 9 years, 1 month ago

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 9 years, 1 month ago

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 9 years, 1 month ago

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 9 years, 1 month ago

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 9 years, 1 month ago

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:: Completed
Change Summary:: Pushed to release-0.8.x (3fc013a)