FileProvider WebApi interaction with file provider account

Review Request #6948 — Created Feb. 14, 2015 and updated

Information

Review Board
master
2ba622f...

Reviewers

This FileProvider module allows users to select files from their online file storage providers to upload directly to ReviewBoard. In order to do so, there has to be way for users to interact with the FileProvider module. This is allowed by implementing webapi calls to interact with the back end. The new webapi integration supports the following:

  • Listing files at the specified path in your file storage
  • Searching for files in your file storage
  • Uploading files from your online file storage to ReviewBoard
  • Fetching a file's meta-information

As well as the general GET, POST, PUT and DELETE functionality for FileProvider object.

Test cases for GET, POST, PUT and DELETE for reviewboard/webapi/resources/file_provider_account.py are written.

Test cases for search, list content and meta-information for reviewboard/webapi/resources/file_provider_node.py are written.

Test cases and some manual tests with the file upload via file provider framework.

Description From Last Updated

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Col: 40 W292 no newline at end of file

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 45 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'rsp' is assigned to but never used

reviewbotreviewbot

Col: 79 W292 no newline at end of file

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Col: 69 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 69 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

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

reviewbotreviewbot

Col: 32 E127 continuation line over-indented for visual indent

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'augment_method_from' imported but unused

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'ObjectDoesNotExist' imported but unused

reviewbotreviewbot

'Dir' imported but unused

reviewbotreviewbot

Col: 18 E203 whitespace before ','

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'FPA' imported but unused

reviewbotreviewbot

Col: 25 E225 missing whitespace around operator

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

Docstrings should be formatted thusly: """A simple one-line summary. Detailed explanation. """

brenniebrennie

Blank line between these.

brenniebrennie

"we turn them all into the correct object" ? "deep copy"

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between tehse.

brenniebrennie

No blank line here.

brenniebrennie

You can use for i, subitem in enumerate(item) here. Then you have access to both the index and the value.

brenniebrennie

Blank line between these.

brenniebrennie

Since you're using an elif, you may want an else: assert False case just to catch any mistakes. (This assumes …

brenniebrennie

Blank line between these.

brenniebrennie

The ObjectDoesNotExist error is a Django queryset-specific error. It is raised when a query cannot find a model instance matching …

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

This documentation needs to be updated to reflect that the file may be provided via a FileProvider and not a …

brenniebrennie

The two request.POST.get calls should be aligned.

brenniebrennie

You can just add file_provider_id and file_provider_path as keyword arguments defaulting to None in the function definition and then you …

brenniebrennie

You don't actually do anything with fields. Did you mean to return it with your INVALID_FORM_DATA response?

brenniebrennie

No blank line here

brenniebrennie

Wrap the conditional in parenthesis and you won't need the backslash.

brenniebrennie

This is a super general exception. In this case, we really want to catch it and log it with its …

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

Since this is for file provider accounts, shouldn't this be called FileProviderAccountResource ?

brenniebrennie

Blank line between these.

brenniebrennie

Trailing comma.

brenniebrennie

Trailing comma

brenniebrennie

Trailing comma

brenniebrennie

Trailing comma.

brenniebrennie

Trailing comma

brenniebrennie

"A JSON blob..."

brenniebrennie

Trailing comma.

brenniebrennie

Trailing comma.

brenniebrennie

"site" -> "provider"

brenniebrennie

"for" -> "from"

brenniebrennie

If you're allowing extra_data on this, you should mention it like the other resources that allow it do.

brenniebrennie

Put the ) on the previous line.

brenniebrennie

Trailing comma.

brenniebrennie

Trailing comma.

brenniebrennie

Trailing comma.

brenniebrennie

Blank line between these.

brenniebrennie

We format generators like so: remove_credential_keys = [ s.strip() for s in remove_credential_keys.split(',') ]

brenniebrennie

Instead of 'Conflicts' perhaps use 'Is mutually exlcusive with' ?

brenniebrennie

This should be FileProviderAccount.DoesNotExist, not the generic Django exception. Each model automatically has a subclass of the generic exception defined. …

brenniebrennie

No blank line here.

brenniebrennie

Alphabetize these.

brenniebrennie

Blank line between these.

brenniebrennie

Trailing comma here and elsewhere.

brenniebrennie

Trailing comma.

brenniebrennie

You can add the search and path as keyword arguments defaulting to None for this function. Then you don't have …

brenniebrennie

"that each specify"

brenniebrennie

'the name of the item and its type (either "file" or "folder")'

brenniebrennie

"will list"

brenniebrennie

Arguments to % formatting should go on the next line.

brenniebrennie

If you are explicitly checking both options, you may want to add an else: assert False case here.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

Col: 80 W291 trailing whitespace

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 29 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

This is operating on self.content, modifying it in the process. That's not safe enough to do. What I'd suggest is …

chipx86chipx86

If the above is done as described, can this all be rolled up into that previous loop, based on the …

chipx86chipx86

"Return"

chipx86chipx86

The purpose of this isn't initially very clear. Might be worth a comment. Same in cases below.

chipx86chipx86

"Return" Same below.

chipx86chipx86

It's best not to reuse the same variable when it's representing different things (in this case, a file path and …

chipx86chipx86

The summary must be a single line.

chipx86chipx86

No need for parens.

chipx86chipx86

This can be one return FileProviderAccount.objects.create statement.

chipx86chipx86

APIs don't return exceptions> They return errors. Same below.

chipx86chipx86

Missing a space before the end of the string.

chipx86chipx86

"or with the file provider framework" could maybe be worded better. How about "..., or will send fields locating the …

chipx86chipx86

"ID" instead of "id".

chipx86chipx86

"Should not" isn't strong enough. How about "A file cannot be provided if file_provider_account_id or file_provider_path are also provided."

chipx86chipx86

Same sort of wording here. Also, this would be better as: fields['...'] = \ ['...']

chipx86chipx86

Same comment about the wording.

chipx86chipx86

Should have a trailing comma.

chipx86chipx86

"frame' should be "framework". Also, this should pass request=request to the function.

chipx86chipx86

The wrapping might be better as: { 'file_provider_account_id': [ 'No file ...', ], }

chipx86chipx86

Similarly, we should have some differences here. Maybe predefine a string saying "Either path or both file_provider_account_id and file_provider_path are …

chipx86chipx86

"... requirements that need to be filled out and configured ..."

chipx86chipx86

We don't want to mention internal classes here. I'd base the wording off of other resources.

chipx86chipx86

"... specific to ..."

chipx86chipx86

"The information is used to authenticate with the file provider."

chipx86chipx86

extra_data isn't actually a field that gets set in requests. We don't want to include this.

chipx86chipx86

Should have similar wording to the ones I recommended above.

chipx86chipx86

Maybe "Clients can choose ..."

chipx86chipx86

You should raise a ValueError here, in order to reuse the error result below.

chipx86chipx86

Swap these. It's nice not to have a not unnecessarily.

chipx86chipx86

Maybe "Valid credentials were not provided."

chipx86chipx86

In REST APIs, we're dealing with setting fields. The "remove" type of object should not be here. Instead, just have …

chipx86chipx86

This shouldn't be here. Same reason as above.

chipx86chipx86

Make sure to change this when the above is changed.

chipx86chipx86

We use "Deletes" twice: In the summary, and in the first sentence. Maybe we can change that. The second sentence …

chipx86chipx86

Swap these. They should be in alphabetical order.

chipx86chipx86

I don't think we need to say "... that is found." Same below.

chipx86chipx86

Maybe file_node? We usually use rsp as "response", as in a JSON blob.

chipx86chipx86

Do we want path to be optional? Or required?

chipx86chipx86

The wording here needs some work: "The path in the provider representing a directory or file to return information on, …

chipx86chipx86

The description and type are inconsistent. The type should be a bool.

chipx86chipx86

Which filename?

chipx86chipx86

Tuples aren't a good way to structure this. They're too limiting long-term, and harder to consume. Entries should be dictionaries …

chipx86chipx86

".. or are related ..."

chipx86chipx86

I don't know that we want a new error code for this. It means clients have to special-case the error. …

chipx86chipx86

"at the path."

chipx86chipx86

No blank line here.

chipx86chipx86

This is a pretty generic excepetion for throwing and catching here. If anything else were to throw this (list_folder, for …

chipx86chipx86

No blank line here.

chipx86chipx86

No blank line here.

chipx86chipx86

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Should start with a capital letter and end in a period.

daviddavid

Should start with a capital letter and end in a period.

daviddavid

Can we call this variable field_errors?

daviddavid

Wrap the entire string in parens and then you don't need the + \\ (two strings next to each other …

daviddavid

Should be in the imperative mood ("Create")

daviddavid

This should have a docstring.

daviddavid

Should be in the imperative mood ("Update")

daviddavid

Should be in the imperative mood ("Delete")

daviddavid

Should be in the imperative mood.

daviddavid

Should be in the imperative mood.

daviddavid

Should be in the imperative mood.

daviddavid

INVALID_FORM_DATA doesn't seem like the right thing here.

daviddavid

This should have a docstring.

daviddavid

Docstring.

daviddavid

Two blank lines here.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
  2. reviewboard/settings.py (Diff revision 1)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 1)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/webapi/errors.py (Diff revision 1)
     
     
    Col: 40
     W292 no newline at end of file
    
  7. Col: 80
     E501 line too long (84 > 79 characters)
    
  8. Col: 80
     E501 line too long (80 > 79 characters)
    
  9. Col: 80
     E501 line too long (80 > 79 characters)
    
  10. Col: 45
     E126 continuation line over-indented for hanging indent
    
  11. Col: 80
     E501 line too long (83 > 79 characters)
    
  12.  local variable 'rsp' is assigned to but never used
    
  13. Col: 79
     W292 no newline at end of file
    
  14. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
  2. reviewboard/settings.py (Diff revision 2)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. reviewboard/testing/fileprovider.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  7. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
  2. reviewboard/settings.py (Diff revision 3)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 3)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 3)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 3)
     
     
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/testing/fileprovider.py (Diff revision 3)
     
     
    Col: 69
     E502 the backslash is redundant between brackets
    
  7. reviewboard/testing/fileprovider.py (Diff revision 3)
     
     
    Col: 69
     E251 unexpected spaces around keyword / parameter equals
    
  8. reviewboard/testing/fileprovider.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  9. Col: 32
     E127 continuation line over-indented for visual indent
    
  10. Col: 80
     E501 line too long (83 > 79 characters)
    
  11. Col: 80
     E501 line too long (80 > 79 characters)
    
  12.  'augment_method_from' imported but unused
    
  13. Col: 13
     E128 continuation line under-indented for visual indent
    
  14. Col: 13
     E128 continuation line under-indented for visual indent
    
  15. Col: 13
     E128 continuation line under-indented for visual indent
    
  16. Col: 13
     E128 continuation line under-indented for visual indent
    
  17. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
  2. reviewboard/settings.py (Diff revision 4)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 4)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 4)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 4)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
VT
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
  2. reviewboard/settings.py (Diff revision 5)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 5)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_CSS' imported but unused
    
  6.  'ObjectDoesNotExist' imported but unused
    
  7.  'Dir' imported but unused
    
  8. Col: 18
     E203 whitespace before ','
    
  9. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/tests/test_file_provider_file.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/resources/file_provider_file.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
    
    
  2. reviewboard/settings.py (Diff revision 6)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 6)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 6)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 6)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 7)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 7)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 7)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 7)
     
     
     'PIPELINE_CSS' imported but unused
    
  6.  'FPA' imported but unused
    
  7. Col: 25
     E225 missing whitespace around operator
    
  8. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 8)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 8)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 9)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 9)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 9)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 9)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 10)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 10)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 10)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 10)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
brennie
  1. 
      
  2. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Docstrings should be formatted thusly:

    """A simple one-line summary.
    
    Detailed explanation.
    """
    
  3. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  4. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    "we turn them all into the correct object" ?

    "deep copy"

  5. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  6. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  7. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between tehse.

  8. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     

    No blank line here.

  9. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     

    You can use for i, subitem in enumerate(item) here. Then you have access to both the index and the value.

  10. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  11. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Since you're using an elif, you may want an else: assert False case just to catch any mistakes. (This assumes that dict and list are the only such options.)

  12. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  13. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     

    The ObjectDoesNotExist error is a Django queryset-specific error. It is raised when a query cannot find a model instance matching the give queryset.

    It is probably worth it to subclass exception as FileProviderError and, if needed, you can subclass that for a FileNotFoundError (or similar) that is specific to file providers.

  14. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  15. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  16. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     

    Blank line between these.

  17. This documentation needs to be updated to reflect that the file may be provided via a FileProvider and not a file upload.

  18. The two request.POST.get calls should be aligned.

  19. You can just add file_provider_id and file_provider_path as keyword arguments defaulting to None in the function definition and then you don't have to use request.POST every time.

    It will also making your formatting easier because the arguments won't be as long.

  20. reviewboard/webapi/resources/base_file_attachment.py (Diff revision 10)
     
     
     
     
     

    You don't actually do anything with fields. Did you mean to return it with your INVALID_FORM_DATA response?

    1. Yes, used the variable fields with INVALID_FORM_DATA

  21. No blank line here

  22. Wrap the conditional in parenthesis and you won't need the backslash.

  23. This is a super general exception. In this case, we really want to catch it and log it with its traceback.

  24. No blank line here.

  25. No blank line here.

  26. reviewboard/webapi/resources/file_provider.py (Diff revision 10)
     
     
     

    Since this is for file provider accounts, shouldn't this be called FileProviderAccountResource ?

    1. Made the change, and propagated changes to file names, variables, docstrings, etc.

  27. reviewboard/webapi/resources/file_provider.py (Diff revision 10)
     
     
     

    Blank line between these.

  28. Trailing comma.

  29. Trailing comma

  30. Trailing comma

  31. Trailing comma.

  32. Trailing comma

  33. "A JSON blob..."

  34. Trailing comma.

  35. Trailing comma.

  36. "site" -> "provider"

  37. "for" -> "from"

  38. reviewboard/webapi/resources/file_provider.py (Diff revision 10)
     
     
     

    If you're allowing extra_data on this, you should mention it like the other resources that allow it do.

  39. reviewboard/webapi/resources/file_provider.py (Diff revision 10)
     
     
     

    Put the ) on the previous line.

  40. Trailing comma.

  41. Trailing comma.

  42. Trailing comma.

  43. reviewboard/webapi/resources/file_provider.py (Diff revision 10)
     
     
     

    Blank line between these.

  44. reviewboard/webapi/resources/file_provider.py (Diff revision 10)
     
     
     

    We format generators like so:

    remove_credential_keys = [
        s.strip()
        for s in remove_credential_keys.split(',')
    ]
    
  45. Instead of 'Conflicts' perhaps use 'Is mutually exlcusive with' ?

  46. This should be FileProviderAccount.DoesNotExist, not the generic Django exception. Each model automatically has a subclass of the generic exception defined.

    This is another reason why I want to discourage you from using the generic ObjectDoesNotExist error elsewhere. You don't want to accidentally catch the wrong exception and have the wrong error-handling code.

  47. No blank line here.

  48. reviewboard/webapi/resources/file_provider_node.py (Diff revision 10)
     
     
     
     
     

    Alphabetize these.

  49. Blank line between these.

  50. Trailing comma here and elsewhere.

  51. Trailing comma.

  52. You can add the search and path as keyword arguments defaulting to None for this function. Then you don't have to hassle with request.GET.

  53. "that each specify"

  54. 'the name of the item and its type (either "file" or "folder")'

  55. Arguments to % formatting should go on the next line.

  56. reviewboard/webapi/resources/file_provider_node.py (Diff revision 10)
     
     
     
     
     

    If you are explicitly checking both options, you may want to add an else: assert False case here.

  57. Why is this method required? Does the default behaviour cause issues?

    1. It had to be overwritten as the model used for this API endpoint is an object, not a django model. When we call register_resource_for_model, it seems to be meant to map WebApi resources to the respectful model. This alternative method is found being done by other webapi resources such as hosting_service.py and remote_repository.py.

  58. Blank line between these.

  59. Blank line between these.

  60. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 11)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 11)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 11)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 11)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. Col: 80
     W291 trailing whitespace
    
  7. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 12)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 12)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 12)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 12)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 13)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 13)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 13)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 13)
     
     
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/webapi/resources/__init__.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  7. Col: 29
     E126 continuation line over-indented for hanging indent
    
  8. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 14)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 14)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 14)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 14)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 15)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 15)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 15)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 15)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. reviewboard/webapi/tests/urls.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  7. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 16)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 16)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 16)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 16)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 17)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 17)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 17)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 17)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 18)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 18)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 18)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 18)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
chipx86
  1. I know this is a long review, but it's a long change.

    I didn't look at the unit tests too closely. The vast majority of the comments are documentation changes for the public API. There are a few small design notes I have though.

  2. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This is operating on self.content, modifying it in the process. That's not safe enough to do. What I'd suggest is building self.content off of the version in the class, and not copying it first.

    Maybe that can be called DEFAULT_CONTENT at that point.

  3. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     
     
     
     
     
     
     

    If the above is done as described, can this all be rolled up into that previous loop, based on the path content in the version in the class?

  4. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     

    "Return"

  5. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     

    The purpose of this isn't initially very clear. Might be worth a comment.

    Same in cases below.

  6. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     

    "Return"

    Same below.

  7. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     
     
     
     
     

    It's best not to reuse the same variable when it's representing different things (in this case, a file path and then a file handle).

    Also, single quotes are preferable over double quotes when possible.

  8. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     
     
     

    The summary must be a single line.

  9. reviewboard/testing/fileprovider.py (Diff revision 18)
     
     

    No need for parens.

  10. reviewboard/testing/testcase.py (Diff revision 18)
     
     
     
     
     
     
     

    This can be one return FileProviderAccount.objects.create statement.

  11. APIs don't return exceptions> They return errors.

    Same below.

  12. Missing a space before the end of the string.

  13. "or with the file provider framework" could maybe be worded better. How about "..., or will send fields locating the file on a registered file provider."

  14. "ID" instead of "id".

  15. "Should not" isn't strong enough. How about "A file cannot be provided if file_provider_account_id or file_provider_path are also provided."

  16. Same sort of wording here.

    Also, this would be better as:

    fields['...'] = \
        ['...']
    
  17. Same comment about the wording.

  18. Should have a trailing comma.

  19. "frame' should be "framework".

    Also, this should pass request=request to the function.

  20. The wrapping might be better as:

    {
        'file_provider_account_id': [
            'No file ...',
         ],
    }
    
  21. reviewboard/webapi/resources/base_file_attachment.py (Diff revision 18)
     
     
     
     
     
     
     
     
     

    Similarly, we should have some differences here.

    Maybe predefine a string saying "Either path or both file_provider_account_id and file_provider_path are required."

  22. "... requirements that need to be filled out and configured ..."

  23. We don't want to mention internal classes here. I'd base the wording off of other resources.

  24. "... specific to ..."

  25. "The information is used to authenticate with the file provider."

  26. reviewboard/webapi/resources/file_provider_account.py (Diff revision 18)
     
     
     
     
     
     
     
     

    extra_data isn't actually a field that gets set in requests. We don't want to include this.

  27. Should have similar wording to the ones I recommended above.

  28. Maybe "Clients can choose ..."

  29. reviewboard/webapi/resources/file_provider_account.py (Diff revision 18)
     
     
     
     
     
     
     

    You should raise a ValueError here, in order to reuse the error result below.

  30. reviewboard/webapi/resources/file_provider_account.py (Diff revision 18)
     
     
     
     
     

    Swap these. It's nice not to have a not unnecessarily.

  31. Maybe "Valid credentials were not provided."

  32. reviewboard/webapi/resources/file_provider_account.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     

    In REST APIs, we're dealing with setting fields. The "remove" type of object should not be here. Instead, just have "credentials" overwrite.

    Also, as above, we shouldn't reference "FileProviderAccount" in API docs, since it's an internal detail.

  33. reviewboard/webapi/resources/file_provider_account.py (Diff revision 18)
     
     
     
     
     
     

    This shouldn't be here. Same reason as above.

  34. Make sure to change this when the above is changed.

  35. We use "Deletes" twice: In the summary, and in the first sentence. Maybe we can change that.

    The second sentence should probably say "For some file providers, Review Board's authentication token will be removed from the service, preventing the token from being re-used. This is not supported for all services."

  36. Swap these. They should be in alphabetical order.

  37. I don't think we need to say "... that is found."

    Same below.

  38. Maybe file_node? We usually use rsp as "response", as in a JSON blob.

  39. Do we want path to be optional? Or required?

    1. I feel path should be optional, as the default would point to the root. Its like when you open DropBox or C: or other file systems, you'll normally start at the root, and navigate around from there.

    2. To avoid any confusion later on. The API assumes a path of / if none is provided for the input. This path would be used with the fileproviders module, which requires a path for its functions. This would be / (provided by the API by default) or the path submitted by the user or caller gives.

  40. reviewboard/webapi/resources/file_provider_node.py (Diff revision 18)
     
     
     
     
     

    The wording here needs some work:

    "The path in the provider representing a directory or file to return information on, or a search string (if ``search`` is true)."

  41. reviewboard/webapi/resources/file_provider_node.py (Diff revision 18)
     
     
     
     
     

    The description and type are inconsistent. The type should be a bool.

  42. Which filename?

  43. Tuples aren't a good way to structure this. They're too limiting long-term, and harder to consume. Entries should be dictionaries instead, and should be structured like item resources.

  44. ".. or are related ..."

  45. I don't know that we want a new error code for this. It means clients have to special-case the error. I think this should be an INVALID_FORM_DATA error that includes reasonable error message for the search field instead.

  46. No blank line here.

  47. This is a pretty generic excepetion for throwing and catching here. If anything else were to throw this (list_folder, for instance), then the caller will get an unexpected error (one referencing "search" for instance).

  48. No blank line here.

  49. No blank line here.

  50. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider_node.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/resources/file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_account.py
        reviewboard/webapi/tests/test_file_provider_node.py
    
    
  2. reviewboard/settings.py (Diff revision 19)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 19)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 19)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 19)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
VT
Review request changed

Change Summary:

Changed the search fuction for the testing fileproviders to require path as input.

Also, changed the default for get_list path parameter to be /

Commit:

-c9329bcb849844cde0aac86ef57c2ddc988c69bf
+2ba622f0c12cded48d2d34595773ce07c954ee39

Diff:

Revision 20 (+1741 -11)

Show changes

david
  1. 
      
  2. reviewboard/testing/fileprovider.py (Diff revision 20)
     
     

    Should start with a capital letter and end in a period.

  3. reviewboard/testing/fileprovider.py (Diff revision 20)
     
     

    Should start with a capital letter and end in a period.

  4. Note to self: depending on where this lands, these may need to change.

  5. Can we call this variable field_errors?

  6. Wrap the entire string in parens and then you don't need the + \\ (two strings next to each other get automatically concatenated).

  7. Should be in the imperative mood ("Create")

  8. This should have a docstring.

  9. Should be in the imperative mood ("Update")

  10. Should be in the imperative mood ("Delete")

  11. Should be in the imperative mood.

  12. Should be in the imperative mood.

  13. Should be in the imperative mood.

  14. INVALID_FORM_DATA doesn't seem like the right thing here.

  15. This should have a docstring.

  16. reviewboard/webapi/tests/mimetypes.py (Diff revision 20)
     
     

    Two blank lines here.

  17. 
      
Loading...