ExtensionInfo can not parse PKG-INFO with multiline description

Review Request #8001 — Created Feb. 26, 2016 and submitted

Information

Djblets
master

Reviewers

ExtensionInfo can not parse PKG-INFO with multiline description

Testing done also with ReviewBoard

Description From Last Updated

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot

Alphabetize

brenniebrennie

Single quotes on 'PKG-INFO'

brenniebrennie

blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

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

brenniebrennie

Blank line between these.

brenniebrennie

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

brenniebrennie

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

This really should be a constant on the class.

brenniebrennie

Can you reflow this? Each sentence needn't take up a whole line.

brenniebrennie

Blank line between these.

brenniebrennie

Single quotes. Can you move the % to the next line?

brenniebrennie

Single quotes. Can you move the % to the next line?

brenniebrennie

Please add a period at the end.

daviddavid

It would be a little nicer to write this using for..else (avoiding the "decoded" variable): for enc in self.encodings: try: …

daviddavid
IC
IC
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. djblets/extensions/extension.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  3. djblets/extensions/extension.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  4. djblets/extensions/extension.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  5. 
      
IC
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. 
      
brennie
  1. 
      
  2. djblets/extensions/extension.py (Diff revision 2)
     
     
     
    Show all issues

    Alphabetize

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

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

      import locale
      import os
      
  3. djblets/extensions/extension.py (Diff revision 2)
     
     
    Show all issues

    Single quotes on 'PKG-INFO'

  4. djblets/extensions/extension.py (Diff revision 2)
     
     
     
    Show all issues

    blank line between these.

  5. djblets/extensions/extension.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

  6. djblets/extensions/extension.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

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

    1. 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?

    2. 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.

  7. djblets/extensions/extension.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

  8. djblets/extensions/extension.py (Diff revision 2)
     
     
    Show all issues

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

    1. 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.

    2. We should probably go with cleaner, simpler code.

  9. 
      
IC
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. 
      
IC
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. djblets/extensions/extension.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. djblets/extensions/extension.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. 
      
IC
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. 
      
brennie
  1. 
      
  2. djblets/extensions/extension.py (Diff revision 5)
     
     
    Show all issues

    This really should be a constant on the class.

  3. djblets/extensions/extension.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Can you reflow this? Each sentence needn't take up a
    whole line.

  4. djblets/extensions/extension.py (Diff revision 5)
     
     
     
    Show all issues

    Blank line between these.

  5. djblets/extensions/extension.py (Diff revision 5)
     
     
    Show all issues

    Single quotes. Can you move the % to the next line?

  6. djblets/extensions/extension.py (Diff revision 5)
     
     
    Show all issues

    Single quotes. Can you move the % to the next line?

  7. 
      
IC
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. 
      
david
  1. 
      
  2. djblets/extensions/extension.py (Diff revision 6)
     
     
    Show all issues

    Please add a period at the end.

  3. djblets/extensions/extension.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

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

  4. 
      
IC
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/extensions/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/extensions/extension.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
IC
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.8.x (3fc013a)