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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/webapi/errors.py (Diff revision 1)
     
     
    Show all issues
    Col: 40
     W292 no newline at end of file
    
  7. Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  8. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  10. Show all issues
    Col: 45
     E126 continuation line over-indented for hanging indent
    
  11. Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  12. Show all issues
     local variable 'rsp' is assigned to but never used
    
  13. Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  6. reviewboard/testing/fileprovider.py (Diff revision 2)
     
     
    Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/testing/fileprovider.py (Diff revision 3)
     
     
    Show all issues
    Col: 69
     E502 the backslash is redundant between brackets
    
  7. reviewboard/testing/fileprovider.py (Diff revision 3)
     
     
    Show all issues
    Col: 69
     E251 unexpected spaces around keyword / parameter equals
    
  8. reviewboard/testing/fileprovider.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  9. Show all issues
    Col: 32
     E127 continuation line over-indented for visual indent
    
  10. Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  11. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. Show all issues
     'augment_method_from' imported but unused
    
  13. Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  14. Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  15. Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  16. Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  6. Show all issues
     'ObjectDoesNotExist' imported but unused
    
  7. Show all issues
     'Dir' imported but unused
    
  8. Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  6. Show all issues
     'FPA' imported but unused
    
  7. Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 9)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 9)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 9)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 10)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 10)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 10)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  6. 
      
brennie
  1. 
      
  2. reviewboard/testing/fileprovider.py (Diff revision 10)
     
     
     
    Show all issues

    Docstrings should be formatted thusly:

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

    Blank line between these.

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

    "we turn them all into the correct object" ?

    "deep copy"

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between tehse.

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

    No blank line here.

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

    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)
     
     
     
    Show all issues

    Blank line between these.

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

    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)
     
     
     
    Show all issues

    Blank line between these.

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

    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)
     
     
     
    Show all issues

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

  17. Show all issues

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

  18. Show all issues

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

  19. Show all issues

    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)
     
     
     
     
     
    Show all issues

    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. Show all issues

    No blank line here

  22. Show all issues

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

  23. Show all issues

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

  24. Show all issues

    No blank line here.

  25. Show all issues

    No blank line here.

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

    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)
     
     
     
    Show all issues

    Blank line between these.

  28. Show all issues

    Trailing comma.

  29. Show all issues

    Trailing comma

  30. Show all issues

    Trailing comma

  31. Show all issues

    Trailing comma.

  32. Show all issues

    Trailing comma

  33. Show all issues

    "A JSON blob..."

  34. Show all issues

    Trailing comma.

  35. Show all issues

    Trailing comma.

  36. Show all issues

    "site" -> "provider"

  37. Show all issues

    "for" -> "from"

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

    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)
     
     
     
    Show all issues

    Put the ) on the previous line.

  40. Show all issues

    Trailing comma.

  41. Show all issues

    Trailing comma.

  42. Show all issues

    Trailing comma.

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

    Blank line between these.

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

    We format generators like so:

    remove_credential_keys = [
        s.strip()
        for s in remove_credential_keys.split(',')
    ]
    
  45. Show all issues

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

  46. Show all issues

    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. Show all issues

    No blank line here.

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

    Alphabetize these.

  49. Show all issues

    Blank line between these.

  50. Show all issues

    Trailing comma here and elsewhere.

  51. Show all issues

    Trailing comma.

  52. Show all issues

    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. Show all issues

    "that each specify"

  54. Show all issues

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

  55. Show all issues

    "will list"

  56. Show all issues

    Arguments to % formatting should go on the next line.

  57. reviewboard/webapi/resources/file_provider_node.py (Diff revision 10)
     
     
     
     
     
    Show all issues

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

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

  59. Show all issues

    Blank line between these.

  60. Show all issues

    Blank line between these.

  61. 
      
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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 11)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 11)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 11)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  6. Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 12)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 12)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 12)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 13)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 13)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 13)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/webapi/resources/__init__.py (Diff revision 13)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  7. Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 14)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 14)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 14)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 15)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 15)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 15)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  6. reviewboard/webapi/tests/urls.py (Diff revision 15)
     
     
    Show all issues
    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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 16)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 16)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 16)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 17)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 17)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 17)
     
     
    Show all issues
     '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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 18)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 18)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 18)
     
     
    Show all issues
     '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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    "Return"

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

    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)
     
     
    Show all issues

    "Return"

    Same below.

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

    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)
     
     
     
     
    Show all issues

    The summary must be a single line.

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

    No need for parens.

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

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

  11. Show all issues

    APIs don't return exceptions> They return errors.

    Same below.

  12. Show all issues

    Missing a space before the end of the string.

  13. Show all issues

    "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. Show all issues

    "ID" instead of "id".

  15. Show all issues

    "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. Show all issues

    Same sort of wording here.

    Also, this would be better as:

    fields['...'] = \
        ['...']
    
  17. Show all issues

    Same comment about the wording.

  18. Show all issues

    Should have a trailing comma.

  19. Show all issues

    "frame' should be "framework".

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

  20. Show all issues

    The wrapping might be better as:

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

    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. Show all issues

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

  23. Show all issues

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

  24. Show all issues

    "... specific to ..."

  25. Show all issues

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

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

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

  27. Show all issues

    Should have similar wording to the ones I recommended above.

  28. Show all issues

    Maybe "Clients can choose ..."

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

    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)
     
     
     
     
     
    Show all issues

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

  31. Show all issues

    Maybe "Valid credentials were not provided."

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

    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)
     
     
     
     
     
     
    Show all issues

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

  34. Show all issues

    Make sure to change this when the above is changed.

  35. Show all issues

    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. Show all issues

    Swap these. They should be in alphabetical order.

  37. Show all issues

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

    Same below.

  38. Show all issues

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

  39. Show all issues

    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)
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
    Show all issues

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

  42. Show all issues

    Which filename?

  43. Show all issues

    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. Show all issues

    ".. or are related ..."

  45. Show all issues

    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. Show all issues

    "at the path."

  47. Show all issues

    No blank line here.

  48. Show all issues

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

  49. Show all issues

    No blank line here.

  50. Show all issues

    No blank line here.

  51. 
      
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)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 19)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 19)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 19)
     
     
    Show all issues
     '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)
     
     
    Show all issues

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

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

    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. Show all issues

    Can we call this variable field_errors?

  6. Show all issues

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

  7. Show all issues

    Should be in the imperative mood ("Create")

  8. Show all issues

    This should have a docstring.

  9. Show all issues

    Should be in the imperative mood ("Update")

  10. Show all issues

    Should be in the imperative mood ("Delete")

  11. Show all issues

    Should be in the imperative mood.

  12. Show all issues

    Should be in the imperative mood.

  13. Show all issues

    Should be in the imperative mood.

  14. Show all issues

    INVALID_FORM_DATA doesn't seem like the right thing here.

  15. Show all issues

    This should have a docstring.

  16. Show all issues

    Docstring.

  17. reviewboard/webapi/tests/mimetypes.py (Diff revision 20)
     
     
    Show all issues

    Two blank lines here.

  18. 
      
Loading...