FileProvider module implementation with Django

Review Request #6806 — Created Jan. 18, 2015 and updated

Information

Review Board
master
c9561cb...

Reviewers

The FileProvider module allows users to select files from their online file storage providers and choose which files to upload to ReviewBoard for file attachments. This module consists of the following files

  • reviewboard/fileproviders/model.py : the Django model for representing file provider accounts for different users and file providers
  • reviewboard/fileproviders/fileprovider.py : the abstract FileProvider object to interact with the file providers.
  • reviewboard/fileproviders/tree.py : contains a base Node object, which can be used to represent files and directories found in the online file providers.
  • reviewboard/fileproviders/errors.py : contains error objects for the FileProvider framework

Anyone who wishes to support a particular file provider can do so by taking creating a derived class from FileProvider and implement the object's methods as neccessary, as well as creating an entry point for the object under reviewboard.file_providers

Test cases for functions and some object method in reviewboard/fileproviders/tests.py

Description From Last Updated

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

undefined name 'webapi_request_fields'

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

'webapi_response_errors' imported but unused

reviewbotreviewbot

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

I wouldn't name names. I would just say "An interface to a service that provides online file storage."

chipx86chipx86

"FileProvider"

chipx86chipx86

This doesn't really say what this is about. Think of the text from an end-user's perspective. What do they want …

chipx86chipx86

No blank line here.

chipx86chipx86

Are you sure you want to be setting the class variable like this? The only way to read/write it is …

TB tbelaire

The """ should be on the first line when there's just a summary.

chipx86chipx86

Summaries can never wrap. They must be on one line, and the line length cannot exceed 79 characters.

chipx86chipx86

When possible, use single quotes instead of double quotes. We should also rename this function, since list is a reserved …

chipx86chipx86

Perhaps this is supposed to be name from above?

TB tbelaire

Blank line between these. Imports are always in 3 sections (excluding the __future__ imports): Python standard library imports. Third-party modules …

chipx86chipx86

I know older code does this, but newer code can use @cached_property and not have to worry about the hasattr …

chipx86chipx86

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 33 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_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Col: 22 W292 no newline at end of file

reviewbotreviewbot

Col: 44 W292 no newline at end of file

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 62 E231 missing whitespace after ','

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

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

'json_dump' imported but unused

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

'User' imported but unused

reviewbotreviewbot

'get_object_or_none' imported but unused

reviewbotreviewbot

'fp_content_item_mimetype' imported but unused

reviewbotreviewbot

'ExtraDataItemMixin' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

We use single quote strings.

brenniebrennie

How about authenticate?

brenniebrennie

No blank line here.

brenniebrennie

Likewise, how about deauthenticate?

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

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

This should be on the previous line.

brenniebrennie

Blank line between these.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

There should probably be a more specific exception for this (and elsewhere you're raising KeyError). Perhaps something along the lines …

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Since this is a JSON blob, this should have a dict type.

brenniebrennie

Since this is a JSON field, this should have a dict type.

brenniebrennie

Blank line between these.

brenniebrennie

Wrap these in parenthesis to avoid using a backslash.

brenniebrennie

We don't use the Python ternary. Instead use a regular if-else or data = {} if credentials: data = json_load(credentials)

brenniebrennie

We should return what the invalid form data is (e.g., { 'reason': 'The credentials supplied were not parsable.' }).

brenniebrennie

Add some documentation that describes what the resource does in broad terms. Documentation that is added to API resources will …

brenniebrennie

There should be an added_in field that describes the Review Board version the API resource was added in. This should …

brenniebrennie

Use double backticks for things marked as code in restructured text. Here and elsewhere.

brenniebrennie

It should read like `... or folder, e.g., ...

brenniebrennie

This should be on the previous line.

brenniebrennie

No blank line here.

brenniebrennie

Are we safe to assume that all instances of FileProvider will use the forward slash? Should there be something akin …

brenniebrennie

Format it using parenthesis so you don't need the backslash, e.g. if get_meta and search_file or not file_name and (search_file …

brenniebrennie

Like I mentioned elsewhere, KeyError probably isn't specific enough (and might accidentally bubble up from somewhere unintentically.

brenniebrennie

You usually don't want to use just except because this can catch things you probably don't want to catch, e.g. …

brenniebrennie

Blank line between these.

brenniebrennie

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

reviewbotreviewbot

All test methods should have a docstring describing what they are testing. Here and elsewhere.

brenniebrennie

file_path should fit on the previous line.

brenniebrennie

Have you found a case where this is needed? Typically, removing authentication for an account just means throwing away the …

chipx86chipx86

This text ends up getting processed as ReST (ReStructured Text), and in it, single backticks will be interpreted as a …

chipx86chipx86

Probably just filename for consistency with the rest of the codebase.

chipx86chipx86

This should probably reference path now, right?

chipx86chipx86

Let's do path instead of file_path, for consistency. Same elsewhere.

chipx86chipx86

Must be one line.

chipx86chipx86

Must be reworded to be one line.

chipx86chipx86

Blank line between these.

chipx86chipx86

I have a number of comments about this function. Given those comments, I think this can really just be: return …

chipx86chipx86

Why not just take *paths, rather than a leading argument?

chipx86chipx86

Blank line between these. This looks like it's protecting the caller from itself. It should be considered invalid to pass …

chipx86chipx86

What does this do?

chipx86chipx86

Global variables should be defined before all functions.

chipx86chipx86

Must be a single line. Same elsewhere. I'll let you look at the rest :)

chipx86chipx86

These should be able to start on the logging.error() line. You can also pass in entry and e as parameters …

chipx86chipx86

These must all be in alphabetical order.

chipx86chipx86

This information is obvious from looking at the class. Can you instead alter the documentation to describe the point of …

chipx86chipx86

related_name= should probably go on the next line.

chipx86chipx86

This is only in Django 1.7, which we are not using.

chipx86chipx86

Both need to have null=True.

chipx86chipx86

We don't use the if ... else ... form in the codebase (outside of lambdas), as it's less explicit and …

chipx86chipx86

Missing a trailing period.

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

For sys modules, it's best to just import the top-level sys and access functions like sys.getsizeof.

chipx86chipx86

Isn't this the reason join_paths exists?

chipx86chipx86

The """ must be on the same line as the summary, in this case. No blank line after.

chipx86chipx86

This should be a single return FileProviderAccount.objects.create(...) statement. Also, when listing multiple lines of parameters in keyword form, you should …

chipx86chipx86

Maybe start this off at 250, to give more room for repository-related errors.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

This says Client Error, but it's a Server Error.

chipx86chipx86

This should just import json and use json.loads, which will be easier to grep and read.

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

Many of these have trailing commas that shouldn't be added.

chipx86chipx86

I don't see anything adding these errors?

chipx86chipx86

"require"

chipx86chipx86

Feels like "... in order to allow access ..." might be easier to read.

chipx86chipx86

We don't explicitly list paths in our documentation in this way, so we should leave this out. I don't think …

chipx86chipx86

We should have is_mutable_by functions on FileProviderAccount for use by this. Even though the implementation is the same, we want …

chipx86chipx86

The space should go at the end of the line in the string, rather than the beginning.

chipx86chipx86

JSON"

chipx86chipx86

Should use isinstance(data, dict).

chipx86chipx86

I think this should just be an INVALID_FORM_DATA as well. We don't want a unique error code for everything right …

chipx86chipx86

This needs to list the field. See how it's done elsewhere.

chipx86chipx86

Same as above.

chipx86chipx86

What's this?

chipx86chipx86

ame as above.

chipx86chipx86

This loses out on things like count queries and pagination. Instead, get_queryset() should be returning request.user.file_provider_accounts.all(), and this function should …

chipx86chipx86

Same comment regarding where the spaces should go.

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Needs to be file_provider_content_...

chipx86chipx86

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 14 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Should use six.iterkeys(...)

chipx86chipx86

I'd like this to be like the hosting service implementation, where it raises an exception if the key isn't previously …

chipx86chipx86

The file this is in should probably be called file.py, since fileproviders/fileproviderfile.py is a bit redundant. Are we going to …

chipx86chipx86

We should use constants for this. Also, the exception needs to have some useful information about the variable and value …

chipx86chipx86

Can you add a comment about this? It's not clear to me what this is trying to solve. I worry …

chipx86chipx86

Test docstrings must start with "Testing" instead of "Test". All these test-related comments apply throughout the file.

chipx86chipx86

Blank line between these.

chipx86chipx86

You can use assertIn`. We don't provide error messages in the assertions. They make the code a bit too verbose …

chipx86chipx86

You want self.assertRaises

chipx86chipx86

Blank line between these.

chipx86chipx86

True doesn't tell me anything. In cases like this, it's best to use keyword arguments. Same below.

chipx86chipx86

local variable 'length' is assigned to but never used

reviewbotreviewbot

These belong in the same import grouping.

chipx86chipx86

Blank line between these.

chipx86chipx86

Can you add doc comments for each of these? They're in the form of: #: Single-line summary. varname = ... …

chipx86chipx86

I think it's reasonable to standardize on '/' as a path separator everywhere, and to just assume that '/' is …

chipx86chipx86

Let's not default a path. The caller should always be explicit in what path they want to list.

chipx86chipx86

"Return"

chipx86chipx86

Instead of the modules listed as these, do: :py:class:`reviewboard.fileprovider.tree.File` etc. Same below.

chipx86chipx86

"specified path is not found"

chipx86chipx86

use :py:class: around ObjectDoesNotExist.

chipx86chipx86

"Return" Same below.

chipx86chipx86

The "If this feature ..." sentence should probably be its own paragraph, since it's an important thing that should stand …

chipx86chipx86

The "Each file ..." should be its own paragraph.

chipx86chipx86

I don't know that we need to bother with the whole part about the input stream. It's already clear that …

chipx86chipx86

The "If no file ..." should be a new paragraph.

chipx86chipx86

With path_sep going away, you can simplify this to: """Return a path composed from the given directory and file names."""

chipx86chipx86

This can be one statement: return '/%s' % '/'.join([ ... ])

chipx86chipx86

"Register"

chipx86chipx86

You can use logging.exception, and you won't have to pass exc_info=1.

chipx86chipx86

This reads strangely. How about: "If a file provider by that name is not found, None will be returned."

chipx86chipx86

This says it's returning the classes, but the function name says it's the names.

chipx86chipx86

viewkeys is only in Python 2.7. This should use six.iterkeys instead.

chipx86chipx86

"Register" This isn't necessarily a third party's class. Just say "Register a file provider class."

chipx86chipx86

"Unregister" You can remove "from the list of known providers."

chipx86chipx86

This model is meant to be consumed, and shouldn't know how it'll be consumed. Today, it's the API. Tomorrow, other …

chipx86chipx86

Blank line betwen these.

chipx86chipx86

For various reasons, having a JSONField with null=True will cause problems. Instead, use default={}.

chipx86chipx86

This must go last in the class.

chipx86chipx86

I'd maybe swap these.

chipx86chipx86

Needs a docstring.

chipx86chipx86

These should say "Return ..." and be fleshed out to describe the conditions upon which they're accessible/mutable. See the other …

chipx86chipx86

No parens.

chipx86chipx86

This should probably have a tearDown that resets the registered file providers. Also, how about FileProviderRegistrationTests?

chipx86chipx86

Maybe just have one of these outside of the TestCase, since all the tests are repeating the same pattern.

chipx86chipx86

That's kinda weird to assign object to this. How about just None?

chipx86chipx86

Instead of names_get, I'd just call this result.

chipx86chipx86

These two things don't match. This testcase can easily be used for more than the join_path function. The docstring should …

chipx86chipx86

Blank line between these.

chipx86chipx86

This should be done in setUp.

chipx86chipx86

"with a" Same below.

chipx86chipx86

I'd argue that a blank string shouldn't be treated as a valid component of a path. os.path.join will skip any …

chipx86chipx86

This should be more than just initialization. Just keep it as "Unit tests for FileProviderFile." Also, missing period.

chipx86chipx86

Blank line between these.

chipx86chipx86

This should be done in setUp.

chipx86chipx86

Blank line between these.

chipx86chipx86

This is about FileProviderDir, but is in the FileProviderFile tests.

chipx86chipx86

Should have a trailing comma. Same below.

chipx86chipx86

Blank line between these. Same below.

chipx86chipx86

"a Node." This shouldn't have any idea of how it's going to be subclassed and used.

chipx86chipx86

This is pretty thick and verbose. I'd have a paragraph per parameter. Also, some other comments: file_provider should be the …

chipx86chipx86

We do the strip a lot. How about instead, you add a normalize_path function to FileProvider.

chipx86chipx86

All paths should be required to be absolute (start with '/'). Once that condition can be guaranteed, you don't need …

chipx86chipx86

Use single quotes when possible.

chipx86chipx86

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 36 E201 whitespace after '('

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

"with the file provider."

chipx86chipx86

I still think we should be removing this.

chipx86chipx86

This all feels like it's getting complicated again, trying to adjust for bad programming. We don't need to handle users …

chipx86chipx86

I think this is all just too complicated as well. I know this came up in Slack at one point, …

chipx86chipx86

Same. This really should just strip. It's not about protecting the caller here either, but rather about having a normalized …

chipx86chipx86

These calls will always perform a database query, even if we don't need it. It's best to avoid this. We …

chipx86chipx86

"Unit tests"

chipx86chipx86

This needs to call the parent tearDown().

chipx86chipx86

"Unit tests"

chipx86chipx86

This needs to call the parent setUp().

chipx86chipx86

Also needs to call the parent.

chipx86chipx86

This should probably be written to be more obvious, and to not set up a generator. I think that'll actually …

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

pass isn't required on empty classes if there's a docstring.

daviddavid

Should be in the imperative mood ("Normalize")

daviddavid

I don't think we want to have defaults like this, because every instance of the model will have the same …

daviddavid

Should be in the imperative mood ("Return")

daviddavid

Should be in the imperative mood ("Return")

daviddavid

Should be in the imperative mood ("Return")

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
    1. You shouldn't be dropping these issues from Review Bot without reason. Code generally has to pass linting to be landed.

  3. Col: 1
     E302 expected 2 blank lines, found 1
    
  4. Col: 5
     E265 block comment should start with '# '
    
  5. Col: 5
     E265 block comment should start with '# '
    
  6. Col: 5
     E303 too many blank lines (2)
    
  7.  undefined name 'webapi_request_fields'
    
  8. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Col: 1
     E302 expected 2 blank lines, found 1
    
  4. Col: 1
     E302 expected 2 blank lines, found 1
    
  5. Col: 1
     E302 expected 2 blank lines, found 1
    
  6. Col: 1
     E302 expected 2 blank lines, found 1
    
  7.  'webapi_response_errors' imported but unused
    
  8. Col: 5
     E265 block comment should start with '# '
    
  9. Col: 5
     E303 too many blank lines (2)
    
  10. Col: 13
     E128 continuation line under-indented for visual indent
    
  11. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Col: 1
     E302 expected 2 blank lines, found 1
    
  4. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/fileprovidersvcs/fileprovider.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/fileprovidersvcs/models.py
        reviewboard/webapi/resources/file_provider.py
    
    Ignored Files:
        reviewboard/fileprovidersvcs/tests.py
        reviewboard/fileprovidersvcs/__init__.py
    
    
  2. 
      
TB
  1. 
      
  2. Are you sure you want to be setting the class variable like this? The only way to read/write it is through FileProvider.name, and there isn't a copy of it for each object.
    I did a quick scan through your changes, and I didn't see name being used, and you are only subclassing object, so I really don't see how this is helpful, but it's possible I'm totally wrong, but could you please explain why it's set this way?

    1. The name attribute can be accessed through all instances of FileProvider. Considering this is a generic base class for file providers such as Dropbox, I imagine that a DropboxFileProvider that inherits from FileProvider might override this field to be 'Dropbox' or similar.

    2. Sure, but is it expected to happen that you have a class as the variable, and need to get the name? Or an instance of the class?
      Really, the only thing you can do with the instance of the class is instanciate it, or hand it off to someone else to do so.
      Because having DropboxFileProvide.name give me dropbox isn't actually telling me much I didn't already know, and couldn't replace with "Dropbox".

      Again, it's possible you are going to implement some metaclass stuff on top, or other extra-ordinary cases, in which case, please ignore my objections, and just put a small comment.

    3. It is also useful in the case to show in e.g., an admin panel the names of all hosting providers available. In this case, we would be getting the hosting providers from by iterating entry points.

    4. Alright, though wouldn't it be easier to iterate over the instances instead of the instances' classes?

    5. I'm not sure about the last point made. The class can be extended to be used for any other file provider services. However in order for RB to know which classes are available for usage, there's a list of entry points populated in Reviewboard.egg-info/entry_points.txt, or a someone calls register_file_provider with the name of the class and the class itself. This forms a name:class pairing in a dictionary. Then, whenever anyone wants to use a FileProvider, the model (from Django DB) can look up in the dictionary, get the class, and instantiate it using the model's information, and make any calls with it.

  3. Perhaps this is supposed to be name from above?

  4. 
      
chipx86
  1. Good start. I have a few stylistic comments that are applicable throughout the change.

    Along with this, I'd recommend we call the directory "fileproviders". I can see that it was based on "hostingsvcs," but that's just shorthand for "hosting services." "File provider services" are a bit long/redundant.

  2. I wouldn't name names. I would just say "An interface to a service that provides online file storage."

  3. "FileProvider"

  4. reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4)
     
     
     
     

    This doesn't really say what this is about. Think of the text from an end-user's perspective. What do they want to know? Maybe something like:

    "File providers allow users to easily authenticate with their online storage services and attach files to review requests."

    1. I rewrote this line to match and changed the following comment to this

      "File providers allow users to easily authenticate with their online file
      storage services, and attach files to review requests.
      
      Subclasses derived can be configured to work with different file providers
      and their APIs to perform operations needed for a FileProvider."
      
  5. reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4)
     
     
     
     

    No blank line here.

  6. The """ should be on the first line when there's just a summary.

  7. Summaries can never wrap. They must be on one line, and the line length cannot exceed 79 characters.

  8. When possible, use single quotes instead of double quotes.

    We should also rename this function, since list is a reserved word.

    1. Is list a reserved word for objects?

  9. reviewboard/fileprovidersvcs/models.py (Diff revision 4)
     
     
     

    Blank line between these.

    Imports are always in 3 sections (excluding the __future__ imports):

    1. Python standard library imports.
    2. Third-party modules (from the perspective of the module doing the import).
    3. In-project imports.
  10. reviewboard/fileprovidersvcs/models.py (Diff revision 4)
     
     
     
     
     
     
     
     

    I know older code does this, but newer code can use @cached_property and not have to worry about the hasattr stuff. See Django's docs on that.

  11. 
      
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/fileproviders/models.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.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/fileproviders/models.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.py
    
    
  2. Col: 5
     E303 too many blank lines (2)
    
    1. Fixes to the styling will appear in next push.

  3. Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/fileproviders/models.py (Diff revision 5)
     
     
    Col: 33
     W292 no newline at end of file
    
  5. reviewboard/settings.py (Diff revision 5)
     
     
     'django_reset' imported but unused
    
    1. Should these settings.py issues be dropped or marked as fixed?

  6. reviewboard/settings.py (Diff revision 5)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  7. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_CSS' imported but unused
    
  8. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_JS' imported but unused
    
  9. reviewboard/testing/fileprovider.py (Diff revision 5)
     
     
    Col: 22
     W292 no newline at end of file
    
  10. reviewboard/webapi/errors.py (Diff revision 5)
     
     
    Col: 44
     W292 no newline at end of file
    
  11. Col: 80
     E501 line too long (82 > 79 characters)
    
  12. Col: 80
     E501 line too long (80 > 79 characters)
    
  13. Col: 62
     E231 missing whitespace after ','
    
  14. Col: 5
     E303 too many blank lines (2)
    
  15. 
      
VT
  1. 
      
  2. reviewboard/fileproviders/models.py (Diff revision 5)
     
     
     
     
     
     

    Import order addressed on my end, same for reviewboard/webapi/tests/test_file_provider.py

  3. 
      
VT
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/fileproviders/models.py
        reviewboard/webapi/resources/file_provider_content.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/test_file_provider_content.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.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/fileproviders/models.py
        reviewboard/webapi/resources/file_provider_content.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/test_file_provider_content.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.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. reviewboard/testing/fileprovider.py (Diff revision 6)
     
     
     'json_dump' imported but unused
    
  7. reviewboard/testing/fileprovider.py (Diff revision 6)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  8. Col: 5
     E303 too many blank lines (2)
    
  9. Col: 5
     E303 too many blank lines (2)
    
  10. Col: 25
     E127 continuation line over-indented for visual indent
    
  11. Col: 1
     E302 expected 2 blank lines, found 1
    
  12. Col: 80
     E501 line too long (89 > 79 characters)
    
  13.  'User' imported but unused
    
  14.  'get_object_or_none' imported but unused
    
  15.  'fp_content_item_mimetype' imported but unused
    
  16.  'ExtraDataItemMixin' imported but unused
    
  17. Col: 1
     E302 expected 2 blank lines, found 1
    
  18. Col: 80
     E501 line too long (89 > 79 characters)
    
  19. 
      
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/fileproviders/models.py
        reviewboard/webapi/resources/file_provider_content.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/test_file_provider_content.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.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/fileproviders/models.py
        reviewboard/webapi/resources/file_provider_content.py
        reviewboard/webapi/errors.py
        reviewboard/testing/fileprovider.py
        reviewboard/webapi/tests/test_file_provider_content.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/root.py
        reviewboard/webapi/tests/test_file_provider.py
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.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. Col: 80
     E501 line too long (81 > 79 characters)
    
    1. This particular issue is now fixed on my end, would be in next commit.

  7. 
      
brennie
  1. 
      
  2. We use single quote strings.

  3. How about authenticate?

  4. No blank line here.

  5. Likewise, how about deauthenticate?

  6. No blank line here.

  7. No blank line here.

  8. No blank line here.

  9. No blank line here.

  10. No blank line here.

  11. No blank line here.

  12. No blank line here.

  13. No blank line here.

  14. No blank line here.

  15. reviewboard/fileproviders/models.py (Diff revision 7)
     
     

    No blank line here.

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

    This should be on the previous line.

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

    Blank line between these.

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

    No blank line here.

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

    No blank line here.

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

    Blank line between these.

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

    Blank line between these.

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

    No blank line here.

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

    No blank line here.

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

    Blank line between these.

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

    Blank line between these.

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

    There should probably be a more specific exception for this (and elsewhere you're raising KeyError). Perhaps something along the lines of a file not found error.

    Also, if the file doesn't have metadata, shouldn't it return None or an empty dictionary instead?

    1. Changed the return model to FileProviderFile.

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

    No blank line here.

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

    No blank line here.

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

    Blank line between these.

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

    Blank line between these.

  31. Since this is a JSON blob, this should have a dict type.

  32. Since this is a JSON field, this should have a dict type.

  33. Blank line between these.

  34. Wrap these in parenthesis to avoid using a backslash.

  35. We don't use the Python ternary. Instead use a regular if-else or

    data = {}
    
    if credentials:
        data = json_load(credentials)
    
  36. We should return what the invalid form data is (e.g., { 'reason': 'The credentials supplied were not parsable.' }).

  37. Add some documentation that describes what the resource does in broad terms. Documentation that is added to API resources will be placed in the generated API documentation.

  38. There should be an added_in field that describes the Review Board version the API resource was added in. This should probably be '3.0'.

  39. Use double backticks for things marked as code in restructured text. Here and elsewhere.

  40. It should read like
    `... or folder, e.g., ...

  41. This should be on the previous line.

  42. No blank line here.

  43. Are we safe to assume that all instances of FileProvider will use the forward slash? Should there be something akin to os.path.join and os.path.sep on each FileProvider instance?

    1. After a bit of consideration, removed this logic from the webapi and let the raw input for the paths be handled by the FileProvider. I feel each one should know how to handle its own input as opposed to a general one-solution that may not fit every one. As a result, FileProvider has a new function join_path and a new self.path_sep.

  44. Format it using parenthesis so you don't need the backslash, e.g.

            if get_meta and search_file or not file_name and (search_file or
                                                              get_meta):
                return INVALID_FORM_DATA                                                          
    

    This is also probably a complex enough if statement to warrant some comments and probably should return a 'reason' field in the error notification.

    1. This logic was removed. There's only search and get folder contents now, a simpler if-else scenario.

  45. Like I mentioned elsewhere, KeyError probably isn't specific enough (and might accidentally bubble up from somewhere unintentically.

  46. You usually don't want to use just except because this can catch things you probably don't want to catch, e.g. KeyboardInterrupt.

    You should probably use except Exception.

    This is also something that should be logged with
    logging.error. Pass the exc_info=1 parameter and it will automatically log the exception information.

  47. Blank line between these.

  48. All test methods should have a docstring describing what they are testing. Here and elsewhere.

  49. reviewboard/webapi/tests/urls.py (Diff revision 7)
     
     
     

    file_path should fit on the previous line.

  50. 
      
VT
reviewbot
  1. 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/fileproviders/models.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
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/settings.py
        reviewboard/webapi/resources/file_provider.py
        reviewboard/fileproviders/models.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
        reviewboard/fileproviders/fileprovider.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
        reviewboard/fileproviders/tests.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_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_JS' imported but unused
    
  6. Col: 80
     E501 line too long (80 > 79 characters)
    
    1. This and the next issue would be resolved in the next iteration. Either the comment block or the entire thing is removed.

  7. Col: 80
     E501 line too long (80 > 79 characters)
    
  8. Col: 80
     E501 line too long (82 > 79 characters)
    
    1. Fixed on my end. Would be pushed in next iteration.

  9. Col: 80
     E501 line too long (81 > 79 characters)
    
  10. Col: 1
     E302 expected 2 blank lines, found 1
    
  11. 
      
chipx86
  1. General note on the description. Instead of putting updates there, you should leave the description as a comprehensive description of the change, and put all updates in the green banner for the draft .when you update the diff.

    1. Also, a lot of these comments are applicable to the rest of the change, but I didn't want to repeat myself too much and build up a long list for you :)

      Another general comment: It's best not to build upon one big change over and over. Instead, having smaller changes for individual pieces means that the review process can be a lot smoother and shorter.

      I'd be fine with that in this particular case. The changes are looking great, but this might be a tough one to review as you continue with it. So my suggestion is this:

      1. Fix up these errors.
      2. Create a new branch containing the code specifically in reviewboard/fileproviders/ (the model, FileProvider, tests). Post just that branch for review on this review request. This is a great standalone one that can be reviewed and landed on a branch. Make sure the description covers that this is the backend for file providers.
      3. Post a new review request containing all the API work, and mark it as depending on this one. That should be a separate branch as well.
  2. Have you found a case where this is needed? Typically, removing authentication for an account just means throwing away the credentials, which in our case would involve deleting the model instance.

  3. This text ends up getting processed as ReST (ReStructured Text), and in it, single backticks will be interpreted as a sort of in-document reference. I'm not sure we need backticks here.

    Same notes apply elsewhere.

  4. Probably just filename for consistency with the rest of the codebase.

  5. This should probably reference path now, right?

  6. Let's do path instead of file_path, for consistency.

    Same elsewhere.

  7. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     

    Must be one line.

  8. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     

    Must be reworded to be one line.

  9. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     

    Blank line between these.

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

    I have a number of comments about this function. Given those comments, I think this can really just be:

    return self.path_sep.join([
        path.strip(self.path_sep)
        for path in paths
    ]) or self.path_sep
    
  11. Why not just take *paths, rather than a leading argument?

  12. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     

    Blank line between these.

    This looks like it's protecting the caller from itself. It should be considered invalid to pass non-strings or strings with invalid characters (like whitespace), rather than working around it. We could be hiding potential problems from the caller until later this way.

  13. What does this do?

    1. It was similar to the issue/idea above. I was trying to protect the caller if they entered something like /, folder1, /pic.jpg as input to the function. Conformed to a variant of the new function.

  14. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     

    Global variables should be defined before all functions.

  15. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     
     

    Must be a single line.

    Same elsewhere. I'll let you look at the rest :)

  16. reviewboard/fileproviders/fileprovider.py (Diff revision 8)
     
     
     
     

    These should be able to start on the logging.error() line.

    You can also pass in entry and e as parameters to logging.error().

    Can you also pass exc_info=1 to logging.error(), so that we can get detailed information when something goes wrong?

  17. reviewboard/fileproviders/models.py (Diff revision 8)
     
     
     
     
     
     

    These must all be in alphabetical order.

  18. reviewboard/fileproviders/models.py (Diff revision 8)
     
     

    This information is obvious from looking at the class. Can you instead alter the documentation to describe the point of the class's existence?

  19. reviewboard/fileproviders/models.py (Diff revision 8)
     
     

    related_name= should probably go on the next line.

  20. reviewboard/fileproviders/models.py (Diff revision 8)
     
     

    This is only in Django 1.7, which we are not using.

  21. reviewboard/fileproviders/models.py (Diff revision 8)
     
     
     

    Both need to have null=True.

  22. reviewboard/fileproviders/models.py (Diff revision 8)
     
     

    We don't use the if ... else ... form in the codebase (outside of lambdas), as it's less explicit and easier to misread. Can you change this to an explicit set of if/else blocks?

  23. reviewboard/fileproviders/models.py (Diff revision 8)
     
     

    Missing a trailing period.

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

    For sys modules, it's best to just import the top-level sys and access functions like sys.getsizeof.

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

    Isn't this the reason join_paths exists?

  26. reviewboard/testing/testcase.py (Diff revision 8)
     
     
     
     
     

    The """ must be on the same line as the summary, in this case.

    No blank line after.

  27. reviewboard/testing/testcase.py (Diff revision 8)
     
     
     
     
     
     

    This should be a single return FileProviderAccount.objects.create(...) statement.

    Also, when listing multiple lines of parameters in keyword form, you should have only one parameter per line.

  28. reviewboard/webapi/errors.py (Diff revision 8)
     
     

    Maybe start this off at 250, to give more room for repository-related errors.

  29. reviewboard/webapi/errors.py (Diff revision 8)
     
     

    Missing a trailing period.

  30. reviewboard/webapi/errors.py (Diff revision 8)
     
     
     
     
     

    This says Client Error, but it's a Server Error.

  31. This should just import json and use json.loads, which will be easier to grep and read.

  32. reviewboard/webapi/resources/file_provider.py (Diff revision 8)
     
     
     
     

    Must be in alphabetical order.

  33. reviewboard/webapi/resources/file_provider.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     

    Many of these have trailing commas that shouldn't be added.

  34. I don't see anything adding these errors?

  35. Feels like "... in order to allow access ..." might be easier to read.

  36. We don't explicitly list paths in our documentation in this way, so we should leave this out. I don't think this paragraph really helps in this situation.

  37. reviewboard/webapi/resources/file_provider.py (Diff revision 8)
     
     
     
     
     
     

    We should have is_mutable_by functions on FileProviderAccount for use by this. Even though the implementation is the same, we want a concept of "can this FileProviderAccount by changed by this user."

  38. The space should go at the end of the line in the string, rather than the beginning.

  39. Should use isinstance(data, dict).

  40. I think this should just be an INVALID_FORM_DATA as well. We don't want a unique error code for everything right now.

  41. This needs to list the field. See how it's done elsewhere.

  42. Same as above.

  43. What's this?

    1. Left that there for a future case where someone would create accounts with conflicting account_name and provider_name, haven't implemented yet.

    2. Implemented, can be found in rr/6948 with the error added in rr/6968

  44. ame as above.

  45. reviewboard/webapi/resources/file_provider.py (Diff revision 8)
     
     
     
     
     
     

    This loses out on things like count queries and pagination. Instead, get_queryset() should be returning request.user.file_provider_accounts.all(), and this function should be removed to allow the parent to be used.

  46. Same comment regarding where the spaces should go.

  47. reviewboard/webapi/tests/mimetypes.py (Diff revision 8)
     
     
     

    Needs to be file_provider_content_...

  48. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. reviewboard/fileproviders/models.py (Diff revision 9)
     
     
    Col: 14
     W291 trailing whitespace
    
  3. 
      
VT
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
        reviewboard/fileproviders/fileproviderfile.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
        reviewboard/fileproviders/fileproviderfile.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. Col: 21
     E126 continuation line over-indented for hanging indent
    
  3. Col: 17
     E126 continuation line over-indented for hanging indent
    
  4. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
        reviewboard/fileproviders/fileproviderfile.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
        reviewboard/fileproviders/fileproviderfile.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
chipx86
  1. Many of my comments (particularly for the unit tests) apply in several places.

  2. Should use six.iterkeys(...)

  3. reviewboard/fileproviders/fileprovider.py (Diff revision 11)
     
     
     

    I'd like this to be like the hosting service implementation, where it raises an exception if the key isn't previously registered.

  4. The file this is in should probably be called file.py, since fileproviders/fileproviderfile.py is a bit redundant.

    Are we going to have something similar for directotries, or other types of information? Perhaps tree.py would be more appropriate.

  5. reviewboard/fileproviders/fileproviderfile.py (Diff revision 11)
     
     
     
     

    We should use constants for this.

    Also, the exception needs to have some useful information about the variable and value not being correct.

  6. reviewboard/fileproviders/fileproviderfile.py (Diff revision 11)
     
     
     
     
     
     

    Can you add a comment about this? It's not clear to me what this is trying to solve.

    I worry about over-compensating for badly-designed data returned by file providers.

    1. You can create an FileProviderFile, you need to submit a name, and a path could be optional. If someone enters name='picFolder/1.jpg', path='' it'll reassign to following name='1.jpg', path='picFolder. Another example is if someone enters name='picFolder/1.jpg', path='documents' it'll reassign to the following name='1.jpg', path='documents/picFolder. I just realized the second example may cause some over-compensating for errors, but I just want to run by you and make sure.

    2. Moved to an initializer that takes in a single path, and splits it to self.name and self.dir appropriately.

  7. reviewboard/fileproviders/tests.py (Diff revision 11)
     
     

    Test docstrings must start with "Testing" instead of "Test".

    All these test-related comments apply throughout the file.

  8. reviewboard/fileproviders/tests.py (Diff revision 11)
     
     
     

    Blank line between these.

  9. reviewboard/fileproviders/tests.py (Diff revision 11)
     
     

    You can use assertIn`.

    We don't provide error messages in the assertions. They make the code a bit too verbose and aren't really that useful in practice. I'd remove them from all tests.

  10. reviewboard/fileproviders/tests.py (Diff revision 11)
     
     
     
     
     
     
     
     

    You want self.assertRaises

  11. reviewboard/fileproviders/tests.py (Diff revision 11)
     
     
     

    Blank line between these.

  12. reviewboard/fileproviders/tests.py (Diff revision 11)
     
     

    True doesn't tell me anything. In cases like this, it's best to use keyword arguments.

    Same below.

  13. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. reviewboard/fileproviders/tests.py (Diff revision 12)
     
     
     local variable 'length' is assigned to but never used
    
  3. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
chipx86
  1. There's a lot of comments here, but most are just documentation-related. That's a good thing! Getting into the low-hanging fruit and out of the big design stuff. You should be able to get through most of these pretty quickly.

  2. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     
     

    These belong in the same import grouping.

  3. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     

    Blank line between these.

  4. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     
     

    Can you add doc comments for each of these? They're in the form of:

    #: Single-line summary.
    varname = ...
    
    #: Single-line summary.
    #:
    #: Multi-line
    #: description.
    varname = ...
    
  5. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     

    I think it's reasonable to standardize on '/' as a path separator everywhere, and to just assume that '/' is the root. No need to define variables. It'll just make things more complicated later.

  6. Let's not default a path. The caller should always be explicit in what path they want to list.

  7. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     

    Instead of the modules listed as these, do:

    :py:class:`reviewboard.fileprovider.tree.File`
    

    etc.

    Same below.

  8. "specified path is not found"

  9. use :py:class: around ObjectDoesNotExist.

  10. "Return"

    Same below.

  11. The "If this feature ..." sentence should probably be its own paragraph, since it's an important thing that should stand out.

  12. The "Each file ..." should be its own paragraph.

  13. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     
     

    I don't know that we need to bother with the whole part about the input stream. It's already clear that a Django File instance must be returned, with all the requirements that come with that.

  14. The "If no file ..." should be a new paragraph.

  15. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     
     
     

    With path_sep going away, you can simplify this to:

    """Return a path composed from the given directory and file names."""
    
  16. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     
     
     
     

    This can be one statement:

    return '/%s' % '/'.join([
        ...
    ])
    
  17. "Register"

  18. reviewboard/fileproviders/fileprovider.py (Diff revision 15)
     
     
     

    You can use logging.exception, and you won't have to pass exc_info=1.

  19. This reads strangely. How about: "If a file provider by that name is not found, None will be returned."

  20. This says it's returning the classes, but the function name says it's the names.

  21. viewkeys is only in Python 2.7. This should use six.iterkeys instead.

  22. "Register"

    This isn't necessarily a third party's class. Just say "Register a file provider class."

  23. "Unregister"

    You can remove "from the list of known providers."

  24. reviewboard/fileproviders/models.py (Diff revision 15)
     
     
     
     

    This model is meant to be consumed, and shouldn't know how it'll be consumed. Today, it's the API. Tomorrow, other things might use this. We should avoid saying how a model will be used, but rather what it's there for (storing information needed to authenticate and communicate with an online file provider).

  25. reviewboard/fileproviders/models.py (Diff revision 15)
     
     
     

    Blank line betwen these.

  26. reviewboard/fileproviders/models.py (Diff revision 15)
     
     
     

    For various reasons, having a JSONField with null=True will cause problems. Instead, use default={}.

  27. reviewboard/fileproviders/models.py (Diff revision 15)
     
     
     

    This must go last in the class.

  28. reviewboard/fileproviders/models.py (Diff revision 15)
     
     

    I'd maybe swap these.

  29. reviewboard/fileproviders/models.py (Diff revision 15)
     
     

    Needs a docstring.

  30. reviewboard/fileproviders/models.py (Diff revision 15)
     
     
     
     
     
     
     
     

    These should say "Return ..." and be fleshed out to describe the conditions upon which they're accessible/mutable. See the other equivalent functions in other classs for examples.

  31. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    No parens.

  32. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    This should probably have a tearDown that resets the registered file providers.

    Also, how about FileProviderRegistrationTests?

  33. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     
     

    Maybe just have one of these outside of the TestCase, since all the tests are repeating the same pattern.

  34. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    That's kinda weird to assign object to this. How about just None?

  35. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    Instead of names_get, I'd just call this result.

  36. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    These two things don't match.

    This testcase can easily be used for more than the join_path function. The docstring should just say it's testing FileProvider functions.

  37. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    Blank line between these.

  38. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    This should be done in setUp.

  39. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    "with a"

    Same below.

  40. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    I'd argue that a blank string shouldn't be treated as a valid component of a path.

    os.path.join will skip any blank strings.

  41. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    This should be more than just initialization. Just keep it as "Unit tests for FileProviderFile."

    Also, missing period.

  42. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    Blank line between these.

  43. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    This should be done in setUp.

  44. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    Blank line between these.

  45. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    This is about FileProviderDir, but is in the FileProviderFile tests.

  46. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     

    Should have a trailing comma.

    Same below.

  47. reviewboard/fileproviders/tests.py (Diff revision 15)
     
     
     

    Blank line between these.

    Same below.

  48. reviewboard/fileproviders/tree.py (Diff revision 15)
     
     

    "a Node."

    This shouldn't have any idea of how it's going to be subclassed and used.

  49. reviewboard/fileproviders/tree.py (Diff revision 15)
     
     
     
     
     
     
     

    This is pretty thick and verbose.

    I'd have a paragraph per parameter.

    Also, some other comments:

    • file_provider should be the first parameter (pretty common to have the owner of an object be the first, in these situations).
    • path would be better than node_path.
    • meta_information is better as metadata.
  50. reviewboard/fileproviders/tree.py (Diff revision 15)
     
     

    We do the strip a lot. How about instead, you add a normalize_path function to FileProvider.

  51. reviewboard/fileproviders/tree.py (Diff revision 15)
     
     
     
     
     

    All paths should be required to be absolute (start with '/').

    Once that condition can be guaranteed, you don't need this if statement. You can just do the rsplit.

  52. reviewboard/fileproviders/tree.py (Diff revision 15)
     
     

    Use single quotes when possible.

  53. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. Col: 13
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/fileproviders/tests.py (Diff revision 16)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/fileproviders/tests.py (Diff revision 16)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. reviewboard/fileproviders/tests.py (Diff revision 16)
     
     
    Col: 36
     E201 whitespace after '('
    
  6. reviewboard/fileproviders/tests.py (Diff revision 16)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. reviewboard/fileproviders/tree.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  8. 
      
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
VT
VT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
VT
chipx86
  1. 
      
  2. "with the file provider."

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

    I still think we should be removing this.

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

    This all feels like it's getting complicated again, trying to adjust for bad programming. We don't need to handle users typing in paths. We really should just be requiring code to be forming valid paths to begin with.

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

    I think this is all just too complicated as well. I know this came up in Slack at one point, but I really believe that it's the caller's fault if they're providing a path with, say, nothing between delimiters, or with .., or whatever. Just like a URL, the path needs to be correct to start with. That's got to be the responsibility of the caller.

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

    Same. This really should just strip. It's not about protecting the caller here either, but rather about having a normalized path ready to join new path items to.

  7. reviewboard/fileproviders/models.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     

    These calls will always perform a database query, even if we don't need it. It's best to avoid this.

    We can do that by checking self.user_id against user.pk. That means checking user.is_authenticated() first, though, like:

    return user.is_authenticated() and self.user_id == user.pk
    
  8. reviewboard/fileproviders/tests.py (Diff revision 18)
     
     

    "Unit tests"

  9. reviewboard/fileproviders/tests.py (Diff revision 18)
     
     

    This needs to call the parent tearDown().

  10. reviewboard/fileproviders/tests.py (Diff revision 18)
     
     

    "Unit tests"

  11. reviewboard/fileproviders/tests.py (Diff revision 18)
     
     

    This needs to call the parent setUp().

  12. reviewboard/fileproviders/tests.py (Diff revision 18)
     
     

    Also needs to call the parent.

  13. reviewboard/fileproviders/tree.py (Diff revision 18)
     
     
     

    This should probably be written to be more obvious, and to not set up a generator. I think that'll actually be more heavy than just:

    path_parts = path.rsplit(..., 1)
    
    self.dir = file_provider.normalize_path(parts[0])
    self.name = file_provider.normalize_path(parts[1])
    

    It's more verbose, but we don't create a generator, and the intentions are clear.

  14. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. Col: 1
     W293 blank line contains whitespace
    
  3. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
VT
Review request changed

Change Summary:

Changed the search fuction for fileproviders to require path as input. Also updated search description to be more exact about what it does (find similar things).

Commit:

-3c144d6344d556f7357f841a2ebf46953db335d9
+c9561cb8e1aac994f9b36f79e9cb3167a98dee14

Diff:

Revision 22 (+471)

Show changes

reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/fileproviders/tests.py
        reviewboard/fileproviders/tree.py
        reviewboard/fileproviders/errors.py
        reviewboard/fileproviders/fileprovider.py
        reviewboard/fileproviders/models.py
    
    Ignored Files:
        reviewboard/fileproviders/__init__.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/fileproviders/errors.py (Diff revision 22)
     
     

    pass isn't required on empty classes if there's a docstring.

  3. Should be in the imperative mood ("Normalize")

  4. reviewboard/fileproviders/models.py (Diff revision 22)
     
     
     

    I don't think we want to have defaults like this, because every instance of the model will have the same instance of the dictionary as the default, so if it gets modified, it will infect all the others.

  5. reviewboard/fileproviders/models.py (Diff revision 22)
     
     

    Should be in the imperative mood ("Return")

  6. reviewboard/fileproviders/models.py (Diff revision 22)
     
     

    Should be in the imperative mood ("Return")

  7. reviewboard/fileproviders/models.py (Diff revision 22)
     
     

    Should be in the imperative mood ("Return")

  8. 
      
Loading...