- Download Diff

Summary:

FileProvider module implementation with Django

Review Request #6806 — Created Jan. 18, 2015 and updated May 20, 2015, 12:35 p.m.

Information
Owner:	VTL-Developer
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Blocks:	7124, 6948
Commit:	c9561cb...
Reviewers
Groups:	reviewboard, students
People:

Description

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

Testing Done

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

Issues

Description	From	Last Updated
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 18, 2015, 12:03 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 18, 2015, 12:03 p.m.
Col: 5 E265 block comment should start with '# '	reviewbot	Jan. 18, 2015, 12:03 p.m.
Col: 5 E265 block comment should start with '# '	reviewbot	Jan. 18, 2015, 12:03 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Jan. 18, 2015, 12:03 p.m.
undefined name 'webapi_request_fields'	reviewbot	Jan. 18, 2015, 12:03 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
'webapi_response_errors' imported but unused	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 5 E265 block comment should start with '# '	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 23, 2015, 4:26 p.m.
I wouldn't name names. I would just say "An interface to a service that provides online file storage."	chipx86	Jan. 30, 2015, 2:04 p.m.
"FileProvider"	chipx86	Jan. 30, 2015, 2:04 p.m.
This doesn't really say what this is about. Think of the text from an end-user's perspective. What do they want …	chipx86	Jan. 30, 2015, 2:16 p.m.
No blank line here.	chipx86	Jan. 30, 2015, 2:04 p.m.
Are you sure you want to be setting the class variable like this? The only way to read/write it is …	TB tbelaire	Jan. 25, 2015, 3:11 p.m.
The """ should be on the first line when there's just a summary.	chipx86	Jan. 30, 2015, 2:05 p.m.
Summaries can never wrap. They must be on one line, and the line length cannot exceed 79 characters.	chipx86	Jan. 30, 2015, 2:05 p.m.
When possible, use single quotes instead of double quotes. We should also rename this function, since list is a reserved …	chipx86	Jan. 30, 2015, 2:05 p.m.
Perhaps this is supposed to be name from above?	TB tbelaire	Jan. 25, 2015, 3:11 p.m.
Blank line between these. Imports are always in 3 sections (excluding the __future__ imports): Python standard library imports. Third-party modules …	chipx86	Jan. 30, 2015, 2:16 p.m.
I know older code does this, but newer code can use @cached_property and not have to worry about the hasattr …	chipx86	Jan. 30, 2015, 2:05 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 33 W292 no newline at end of file	reviewbot	Jan. 30, 2015, 2:14 p.m.
'django_reset' imported but unused	reviewbot	Feb. 7, 2015, 10:03 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 7, 2015, 10:03 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 7, 2015, 10:03 a.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 7, 2015, 10:03 a.m.
Col: 22 W292 no newline at end of file	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 44 W292 no newline at end of file	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 62 E231 missing whitespace after ','	reviewbot	Jan. 30, 2015, 2:14 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Jan. 30, 2015, 2:14 p.m.
'django_reset' imported but unused	reviewbot	Feb. 7, 2015, 10:02 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 7, 2015, 10:02 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 7, 2015, 10:02 a.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 7, 2015, 10:02 a.m.
'json_dump' imported but unused	reviewbot	Feb. 7, 2015, 9:59 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 7, 2015, 9:59 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 7, 2015, 9:59 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 7, 2015, 9:59 a.m.
Col: 25 E127 continuation line over-indented for visual indent	reviewbot	Feb. 7, 2015, 10 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 7, 2015, 10 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 7, 2015, 10 a.m.
'User' imported but unused	reviewbot	Feb. 7, 2015, 10 a.m.
'get_object_or_none' imported but unused	reviewbot	Feb. 7, 2015, 10 a.m.
'fp_content_item_mimetype' imported but unused	reviewbot	Feb. 7, 2015, 10 a.m.
'ExtraDataItemMixin' imported but unused	reviewbot	Feb. 7, 2015, 9:59 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 7, 2015, 9:59 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 7, 2015, 9:59 a.m.
We use single quote strings.	brennie	Feb. 9, 2015, 5:18 p.m.
How about authenticate?	brennie	Feb. 9, 2015, 6:13 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:28 p.m.
Likewise, how about deauthenticate?	brennie	Feb. 9, 2015, 6:13 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:28 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:28 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:28 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:28 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
'django_reset' imported but unused	reviewbot	Feb. 7, 2015, 10:02 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 7, 2015, 10:02 a.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 7, 2015, 10:02 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 7, 2015, 10:02 a.m.
This should be on the previous line.	brennie	Feb. 9, 2015, 5:29 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:29 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:29 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:35 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:36 p.m.
There should probably be a more specific exception for this (and elsewhere you're raising KeyError). Perhaps something along the lines …	brennie	Feb. 18, 2015, 12:19 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:36 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:36 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:36 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:37 p.m.
Since this is a JSON blob, this should have a dict type.	brennie	Feb. 9, 2015, 6:15 p.m.
Since this is a JSON field, this should have a dict type.	brennie	Feb. 9, 2015, 6:15 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:38 p.m.
Wrap these in parenthesis to avoid using a backslash.	brennie	Feb. 9, 2015, 5:38 p.m.
We don't use the Python ternary. Instead use a regular if-else or data = {} if credentials: data = json_load(credentials)	brennie	Feb. 9, 2015, 5:38 p.m.
We should return what the invalid form data is (e.g., { 'reason': 'The credentials supplied were not parsable.' }).	brennie	Feb. 14, 2015, 8:31 a.m.
Add some documentation that describes what the resource does in broad terms. Documentation that is added to API resources will …	brennie	Feb. 14, 2015, 8:22 a.m.
There should be an added_in field that describes the Review Board version the API resource was added in. This should …	brennie	Feb. 9, 2015, 6:18 p.m.
Use double backticks for things marked as code in restructured text. Here and elsewhere.	brennie	Feb. 14, 2015, 8:32 a.m.
It should read like `... or folder, e.g., ...	brennie	Feb. 9, 2015, 5:38 p.m.
This should be on the previous line.	brennie	Feb. 9, 2015, 5:38 p.m.
No blank line here.	brennie	Feb. 9, 2015, 5:38 p.m.
Are we safe to assume that all instances of FileProvider will use the forward slash? Should there be something akin …	brennie	Feb. 9, 2015, 5:38 p.m.
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 …	brennie	Feb. 9, 2015, 5:41 p.m.
Like I mentioned elsewhere, KeyError probably isn't specific enough (and might accidentally bubble up from somewhere unintentically.	brennie	Feb. 18, 2015, 12:20 p.m.
You usually don't want to use just except because this can catch things you probably don't want to catch, e.g. …	brennie	Feb. 9, 2015, 5:42 p.m.
Blank line between these.	brennie	Feb. 9, 2015, 5:42 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 7, 2015, 10:01 a.m.
All test methods should have a docstring describing what they are testing. Here and elsewhere.	brennie	Feb. 18, 2015, 12:20 p.m.
file_path should fit on the previous line.	brennie	Feb. 9, 2015, 6:17 p.m.
Have you found a case where this is needed? Typically, removing authentication for an account just means throwing away the …	chipx86	Feb. 20, 2015, 4:26 p.m.
This text ends up getting processed as ReST (ReStructured Text), and in it, single backticks will be interpreted as a …	chipx86	Feb. 14, 2015, 7:58 a.m.
Probably just filename for consistency with the rest of the codebase.	chipx86	Feb. 14, 2015, 6:46 a.m.
This should probably reference path now, right?	chipx86	Feb. 14, 2015, 6:46 a.m.
Let's do path instead of file_path, for consistency. Same elsewhere.	chipx86	Feb. 14, 2015, 6:46 a.m.
Must be one line.	chipx86	Feb. 14, 2015, 6:47 a.m.
Must be reworded to be one line.	chipx86	Feb. 14, 2015, 6:48 a.m.
Blank line between these.	chipx86	Feb. 14, 2015, 6:47 a.m.
I have a number of comments about this function. Given those comments, I think this can really just be: return …	chipx86	Feb. 14, 2015, 8:13 a.m.
Why not just take *paths, rather than a leading argument?	chipx86	Feb. 14, 2015, 8:02 a.m.
Blank line between these. This looks like it's protecting the caller from itself. It should be considered invalid to pass …	chipx86	Feb. 14, 2015, 8:02 a.m.
What does this do?	chipx86	Feb. 14, 2015, 8:33 a.m.
Global variables should be defined before all functions.	chipx86	Feb. 14, 2015, 7 a.m.
Must be a single line. Same elsewhere. I'll let you look at the rest :)	chipx86	Feb. 18, 2015, 12:21 p.m.
These should be able to start on the logging.error() line. You can also pass in entry and e as parameters …	chipx86	Feb. 14, 2015, 7:04 a.m.
These must all be in alphabetical order.	chipx86	Feb. 14, 2015, 7:04 a.m.
This information is obvious from looking at the class. Can you instead alter the documentation to describe the point of …	chipx86	Feb. 14, 2015, 8:19 a.m.
related_name= should probably go on the next line.	chipx86	Feb. 14, 2015, 7:05 a.m.
This is only in Django 1.7, which we are not using.	chipx86	Feb. 14, 2015, 7:05 a.m.
Both need to have null=True.	chipx86	Feb. 20, 2015, 4:26 p.m.
We don't use the if ... else ... form in the codebase (outside of lambdas), as it's less explicit and …	chipx86	Feb. 14, 2015, 7:06 a.m.
Missing a trailing period.	chipx86	Feb. 14, 2015, 7:06 a.m.
'django_reset' imported but unused	reviewbot	Feb. 11, 2015, 1:11 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 11, 2015, 1:11 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 11, 2015, 1:11 p.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 11, 2015, 1:11 p.m.
For sys modules, it's best to just import the top-level sys and access functions like sys.getsizeof.	chipx86	Feb. 14, 2015, 7:08 a.m.
Isn't this the reason join_paths exists?	chipx86	Feb. 14, 2015, 7:08 a.m.
The """ must be on the same line as the summary, in this case. No blank line after.	chipx86	Feb. 14, 2015, 7:10 a.m.
This should be a single return FileProviderAccount.objects.create(...) statement. Also, when listing multiple lines of parameters in keyword form, you should …	chipx86	Feb. 14, 2015, 7:10 a.m.
Maybe start this off at 250, to give more room for repository-related errors.	chipx86	Feb. 14, 2015, 7:12 a.m.
Missing a trailing period.	chipx86	Feb. 14, 2015, 7:12 a.m.
This says Client Error, but it's a Server Error.	chipx86	Feb. 14, 2015, 7:15 a.m.
This should just import json and use json.loads, which will be easier to grep and read.	chipx86	Feb. 14, 2015, 7:17 a.m.
Must be in alphabetical order.	chipx86	Feb. 14, 2015, 7:18 a.m.
Many of these have trailing commas that shouldn't be added.	chipx86	Feb. 14, 2015, 7:19 a.m.
I don't see anything adding these errors?	chipx86	Feb. 14, 2015, 7:45 a.m.
"require"	chipx86	Feb. 14, 2015, 7:20 a.m.
Feels like "... in order to allow access ..." might be easier to read.	chipx86	Feb. 14, 2015, 7:20 a.m.
We don't explicitly list paths in our documentation in this way, so we should leave this out. I don't think …	chipx86	Feb. 14, 2015, 7:20 a.m.
We should have is_mutable_by functions on FileProviderAccount for use by this. Even though the implementation is the same, we want …	chipx86	Feb. 14, 2015, 7:54 a.m.
The space should go at the end of the line in the string, rather than the beginning.	chipx86	Feb. 14, 2015, 7:21 a.m.
JSON"	chipx86	Feb. 14, 2015, 7:23 a.m.
Should use isinstance(data, dict).	chipx86	Feb. 14, 2015, 7:23 a.m.
I think this should just be an INVALID_FORM_DATA as well. We don't want a unique error code for everything right …	chipx86	Feb. 14, 2015, 7:32 a.m.
This needs to list the field. See how it's done elsewhere.	chipx86	Feb. 14, 2015, 8:34 a.m.
Same as above.	chipx86	Feb. 14, 2015, 7:53 a.m.
What's this?	chipx86	Feb. 21, 2015, 5:54 p.m.
ame as above.	chipx86	Feb. 14, 2015, 7:53 a.m.
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 …	chipx86	Feb. 18, 2015, 6:55 a.m.
Same comment regarding where the spaces should go.	chipx86	Feb. 14, 2015, 7:51 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 11, 2015, 1:11 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 11, 2015, 1:11 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 11, 2015, 1:12 p.m.
Needs to be file_provider_content_...	chipx86	Feb. 14, 2015, 7:53 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 11, 2015, 1:12 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 11, 2015, 1:12 p.m.
Col: 14 W291 trailing whitespace	reviewbot	Feb. 18, 2015, 6:54 a.m.
Col: 21 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 18, 2015, 12:58 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 20, 2015, 4:25 p.m.
Should use six.iterkeys(...)	chipx86	Feb. 22, 2015, 5:39 p.m.
I'd like this to be like the hosting service implementation, where it raises an exception if the key isn't previously …	chipx86	Feb. 22, 2015, 5:40 p.m.
The file this is in should probably be called file.py, since fileproviders/fileproviderfile.py is a bit redundant. Are we going to …	chipx86	Feb. 23, 2015, 5:13 p.m.
We should use constants for this. Also, the exception needs to have some useful information about the variable and value …	chipx86	Feb. 23, 2015, 3:47 p.m.
Can you add a comment about this? It's not clear to me what this is trying to solve. I worry …	chipx86	March 5, 2015, 3:40 p.m.
Test docstrings must start with "Testing" instead of "Test". All these test-related comments apply throughout the file.	chipx86	Feb. 22, 2015, 6:33 p.m.
Blank line between these.	chipx86	Feb. 22, 2015, 6:34 p.m.
You can use assertIn`. We don't provide error messages in the assertions. They make the code a bit too verbose …	chipx86	Feb. 22, 2015, 6:34 p.m.
You want self.assertRaises	chipx86	Feb. 22, 2015, 6:34 p.m.
Blank line between these.	chipx86	Feb. 22, 2015, 6:34 p.m.
True doesn't tell me anything. In cases like this, it's best to use keyword arguments. Same below.	chipx86	Feb. 22, 2015, 6:34 p.m.
local variable 'length' is assigned to but never used	reviewbot	Feb. 23, 2015, 5:49 p.m.
These belong in the same import grouping.	chipx86	March 16, 2015, 8:32 a.m.
Blank line between these.	chipx86	March 16, 2015, 8:01 a.m.
Can you add doc comments for each of these? They're in the form of: #: Single-line summary. varname = ... …	chipx86	March 17, 2015, 4:35 p.m.
I think it's reasonable to standardize on '/' as a path separator everywhere, and to just assume that '/' is …	chipx86	March 16, 2015, 8:01 a.m.
Let's not default a path. The caller should always be explicit in what path they want to list.	chipx86	March 16, 2015, 8:36 a.m.
"Return"	chipx86	March 16, 2015, 8:30 a.m.
Instead of the modules listed as these, do: :py:class:`reviewboard.fileprovider.tree.File` etc. Same below.	chipx86	March 17, 2015, 8:07 a.m.
"specified path is not found"	chipx86	March 16, 2015, 8:03 a.m.
use :py:class: around ObjectDoesNotExist.	chipx86	March 17, 2015, 8:07 a.m.
"Return" Same below.	chipx86	March 16, 2015, 8:04 a.m.
The "If this feature ..." sentence should probably be its own paragraph, since it's an important thing that should stand …	chipx86	March 16, 2015, 8:05 a.m.
The "Each file ..." should be its own paragraph.	chipx86	March 16, 2015, 8:07 a.m.
I don't know that we need to bother with the whole part about the input stream. It's already clear that …	chipx86	March 16, 2015, 8:08 a.m.
The "If no file ..." should be a new paragraph.	chipx86	March 16, 2015, 8:08 a.m.
With path_sep going away, you can simplify this to: """Return a path composed from the given directory and file names."""	chipx86	March 16, 2015, 8:37 a.m.
This can be one statement: return '/%s' % '/'.join([ ... ])	chipx86	March 16, 2015, 8:11 a.m.
"Register"	chipx86	March 16, 2015, 8:11 a.m.
You can use logging.exception, and you won't have to pass exc_info=1.	chipx86	March 16, 2015, 8:11 a.m.
This reads strangely. How about: "If a file provider by that name is not found, None will be returned."	chipx86	March 16, 2015, 8:12 a.m.
This says it's returning the classes, but the function name says it's the names.	chipx86	March 16, 2015, 8:14 a.m.
viewkeys is only in Python 2.7. This should use six.iterkeys instead.	chipx86	March 16, 2015, 8:14 a.m.
"Register" This isn't necessarily a third party's class. Just say "Register a file provider class."	chipx86	March 16, 2015, 8:15 a.m.
"Unregister" You can remove "from the list of known providers."	chipx86	March 16, 2015, 8:15 a.m.
This model is meant to be consumed, and shouldn't know how it'll be consumed. Today, it's the API. Tomorrow, other …	chipx86	March 16, 2015, 4:06 p.m.
Blank line betwen these.	chipx86	March 16, 2015, 8:15 a.m.
For various reasons, having a JSONField with null=True will cause problems. Instead, use default={}.	chipx86	March 16, 2015, 8:16 a.m.
This must go last in the class.	chipx86	March 16, 2015, 8:16 a.m.
I'd maybe swap these.	chipx86	March 16, 2015, 8:16 a.m.
Needs a docstring.	chipx86	March 16, 2015, 8:51 a.m.
These should say "Return ..." and be fleshed out to describe the conditions upon which they're accessible/mutable. See the other …	chipx86	March 16, 2015, 8:42 a.m.
No parens.	chipx86	March 16, 2015, 8:17 a.m.
This should probably have a tearDown that resets the registered file providers. Also, how about FileProviderRegistrationTests?	chipx86	March 16, 2015, 8:40 a.m.
Maybe just have one of these outside of the TestCase, since all the tests are repeating the same pattern.	chipx86	March 16, 2015, 8:18 a.m.
That's kinda weird to assign object to this. How about just None?	chipx86	March 16, 2015, 8:19 a.m.
Instead of names_get, I'd just call this result.	chipx86	March 16, 2015, 8:20 a.m.
These two things don't match. This testcase can easily be used for more than the join_path function. The docstring should …	chipx86	March 16, 2015, 8:22 a.m.
Blank line between these.	chipx86	March 16, 2015, 8:22 a.m.
This should be done in setUp.	chipx86	March 16, 2015, 8:22 a.m.
"with a" Same below.	chipx86	March 16, 2015, 8:23 a.m.
I'd argue that a blank string shouldn't be treated as a valid component of a path. os.path.join will skip any …	chipx86	March 16, 2015, 4 p.m.
This should be more than just initialization. Just keep it as "Unit tests for FileProviderFile." Also, missing period.	chipx86	March 16, 2015, 8:24 a.m.
Blank line between these.	chipx86	March 16, 2015, 8:37 a.m.
This should be done in setUp.	chipx86	March 16, 2015, 8:24 a.m.
Blank line between these.	chipx86	March 16, 2015, 8:33 a.m.
This is about FileProviderDir, but is in the FileProviderFile tests.	chipx86	March 16, 2015, 8:33 a.m.
Should have a trailing comma. Same below.	chipx86	March 16, 2015, 8:25 a.m.
Blank line between these. Same below.	chipx86	March 16, 2015, 8:26 a.m.
"a Node." This shouldn't have any idea of how it's going to be subclassed and used.	chipx86	March 16, 2015, 8:26 a.m.
This is pretty thick and verbose. I'd have a paragraph per parameter. Also, some other comments: file_provider should be the …	chipx86	March 16, 2015, 8:44 a.m.
We do the strip a lot. How about instead, you add a normalize_path function to FileProvider.	chipx86	March 17, 2015, 4:34 p.m.
All paths should be required to be absolute (start with '/'). Once that condition can be guaranteed, you don't need …	chipx86	March 16, 2015, 4:06 p.m.
Use single quotes when possible.	chipx86	March 16, 2015, 8:27 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	March 17, 2015, 4:34 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 17, 2015, 4:22 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 17, 2015, 4:22 p.m.
Col: 36 E201 whitespace after '('	reviewbot	March 17, 2015, 4:22 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 17, 2015, 4:22 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 17, 2015, 4:34 p.m.
"with the file provider."	chipx86	April 2, 2015, 6:24 a.m.
I still think we should be removing this.	chipx86	April 2, 2015, 6:24 a.m.
This all feels like it's getting complicated again, trying to adjust for bad programming. We don't need to handle users …	chipx86	April 2, 2015, 6:24 a.m.
I think this is all just too complicated as well. I know this came up in Slack at one point, …	chipx86	April 2, 2015, 6:24 a.m.
Same. This really should just strip. It's not about protecting the caller here either, but rather about having a normalized …	chipx86	April 2, 2015, 6:24 a.m.
These calls will always perform a database query, even if we don't need it. It's best to avoid this. We …	chipx86	April 2, 2015, 6:24 a.m.
"Unit tests"	chipx86	April 2, 2015, 6:24 a.m.
This needs to call the parent tearDown().	chipx86	April 2, 2015, 6:23 a.m.
"Unit tests"	chipx86	April 2, 2015, 6:23 a.m.
This needs to call the parent setUp().	chipx86	April 2, 2015, 6:23 a.m.
Also needs to call the parent.	chipx86	April 2, 2015, 6:23 a.m.
This should probably be written to be more obvious, and to not set up a generator. I think that'll actually …	chipx86	April 2, 2015, 6:23 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	April 2, 2015, 9:21 a.m.
pass isn't required on empty classes if there's a docstring.	david	May 20, 2015, 12:35 p.m.
Should be in the imperative mood ("Normalize")	david	May 20, 2015, 12:35 p.m.
I don't think we want to have defaults like this, because every instance of the model will have the same …	david	May 20, 2015, 12:35 p.m.
Should be in the imperative mood ("Return")	david	May 20, 2015, 12:35 p.m.
Should be in the imperative mood ("Return")	david	May 20, 2015, 12:35 p.m.
Should be in the imperative mood ("Return")	david	May 20, 2015, 12:35 p.m.

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

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

Show all issues

Col: 1
 E302 expected 2 blank lines, found 1

brennie Jan. 19, 2015, 9:15 a.m.

You shouldn't be dropping these issues from Review Bot without reason. Code generally has to pass linting to be landed.

reviewboard/webapi/resources/file_provider.py (Diff revision 1)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/resources/file_provider.py (Diff revision 1)
Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/file_provider.py (Diff revision 1)
Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/file_provider.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/file_provider.py (Diff revision 1)
Show all issues
```
 undefined name 'webapi_request_fields'
```

Description:

	+	Orig: January 18, 2015
	+
		Started working on the File-Provider webapi, model and objects.

		Added the file-provider api to the routes, and started working on pseudo-code on how the calls should work out. Started working on the model and objects for the fileprovider. Basic overview with how the object should work (with NotImplementedError for each function) as well as database schema for the model.
	+
	+	UPDATE: January 23, 2015
	+
	+	Updated the model and fileprovider.py to accomodate for third-party derived classes. Updated the webapi for GET (lists) and POST.

Commit:

-	11efe84b175ba3e9fb5d7893466a6cabfaf00624
+	6f24a5400c0f5fab53851405c8a43537e631420f

Diff:

Revision 2 (+310)

Show changes

	reviewboard/fileprovidersvcs/__init__.py
	reviewboard/fileprovidersvcs/fileprovider.py
	reviewboard/fileprovidersvcs/models.py
	reviewboard/fileprovidersvcs/tests.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/file_provider.py
	reviewboard/webapi/resources/root.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



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

reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/resources/file_provider.py (Diff revision 2)
Show all issues
```
 'webapi_response_errors' imported but unused
```
reviewboard/webapi/resources/file_provider.py (Diff revision 2)
Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/file_provider.py (Diff revision 2)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/file_provider.py (Diff revision 2)
Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Commit:

-	6f24a5400c0f5fab53851405c8a43537e631420f
+	911ca9761925ae5598fc7aa6c6866a6de5ad2aaa

Diff:

Revision 3 (+310)

Show changes

	reviewboard/fileprovidersvcs/__init__.py
	reviewboard/fileprovidersvcs/fileprovider.py
	reviewboard/fileprovidersvcs/models.py
	reviewboard/fileprovidersvcs/tests.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/file_provider.py
	reviewboard/webapi/resources/root.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



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

reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 3)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 3)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Commit:

-	911ca9761925ae5598fc7aa6c6866a6de5ad2aaa
+	67b37678dead45247441c96e4eb01b179f7d7b90

Diff:

Revision 4 (+312)

Show changes

	reviewboard/fileprovidersvcs/__init__.py
	reviewboard/fileprovidersvcs/fileprovider.py
	reviewboard/fileprovidersvcs/models.py
	reviewboard/fileprovidersvcs/tests.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/file_provider.py
	reviewboard/webapi/resources/root.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

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

Show all issues

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?

brennie Jan. 24, 2015, 1:33 p.m.

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.

tbelaire Jan. 24, 2015, 2:59 p.m.

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.

brennie Jan. 25, 2015, 11:27 a.m.

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.

tbelaire Jan. 25, 2015, 3:12 p.m.

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

VTL-Developer Jan. 27, 2015, 8:15 a.m.

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.

reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4)
Show all issues
```
Perhaps this is supposed to be name from above?
```

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.

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

Show all issues

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

reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4)
Show all issues
```
"FileProvider"
```

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

Show all issues

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

VTL-Developer Jan. 29, 2015, 12:46 p.m.

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

reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4)
Show all issues
```
No blank line here.
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

When possible, use single quotes instead of double quotes.

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

VT

VTL-Developer Jan. 29, 2015, 12:46 p.m.
```
Is list a reserved word for objects?
```

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

Show all issues

Blank line between these.

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

Python standard library imports.
Third-party modules (from the perspective of the module doing the import).
In-project imports.

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

Show all issues

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.

Summary:

-	[WIP] Basic layout for fileprovider with Django
+	[WIP] FileProvider implementation with Django

Description:

~		Orig: January 18, 2015
	~	This FileProvider framework allows users to select files from their online file storage providers to upload directly to ReviewBoard. This includes defining the base class that can be subclassed to handle different file providers, and implementing webapi calls to allow users to interact with the back end. The webapi supports the following:
	+
	+	Listing files at the specified path in your file storage
	+	Searching for files in your file storage
	+	Uploading files from your file storage to ReviewBoard
	+	Fetching a file's meta-data
	+
	+	As well as the general GET, POST, PUT and DELETE functionality.
	+
	+	Orignal: January 18, 2015

		Started working on the File-Provider webapi, model and objects.

		Added the file-provider api to the routes, and started working on pseudo-code on how the calls should work out. Started working on the model and objects for the fileprovider. Basic overview with how the object should work (with NotImplementedError for each function) as well as database schema for the model.

		UPDATE: January 23, 2015

		Updated the model and fileprovider.py to accomodate for third-party derived classes. Updated the webapi for GET (lists) and POST.
	+
	+	UPDATE: January 30, 2015
	+
	+	Updated the model to have two JSONfields, and added implementation for GET, POST, DELETE, and PUT requests and their test cases.

Commit:

-	67b37678dead45247441c96e4eb01b179f7d7b90
+	efa4cb93102614f0d5cabeba2746edd0f2deb963

Diff:

Revision 5 (+847 -1)

Show changes

	reviewboard/settings.py
	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/testing/fileprovider.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/errors.py
	6 more

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

reviewboard/fileproviders/fileprovider.py (Diff revision 5)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
1. VT
  
  VTL-Developer Jan. 30, 2015, 2:16 p.m.
  Fixes to the styling will appear in next push.
reviewboard/fileproviders/fileprovider.py (Diff revision 5)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileproviders/models.py (Diff revision 5)
Show all issues
```
Col: 33
 W292 no newline at end of file
```

reviewboard/settings.py (Diff revision 5)

Show all issues

 'django_reset' imported but unused

VTL-Developer Jan. 30, 2015, 2:19 p.m.

Should these settings.py issues be dropped or marked as fixed?

reviewboard/settings.py (Diff revision 5)

Show all issues

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

reviewboard/settings.py (Diff revision 5)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 5)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/testing/fileprovider.py (Diff revision 5)
Show all issues
```
Col: 22
 W292 no newline at end of file
```
reviewboard/webapi/errors.py (Diff revision 5)
Show all issues
```
Col: 44
 W292 no newline at end of file
```
reviewboard/webapi/resources/file_provider.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_file_provider.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/tests/test_file_provider.py (Diff revision 5)
Show all issues
```
Col: 62
 E231 missing whitespace after ','
```
reviewboard/webapi/tests/test_file_provider.py (Diff revision 5)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

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

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

Testing Done:

Test cases made for list (GET, POST) and items (GET, PUT, DELETE)

Description:

		This FileProvider framework allows users to select files from their online file storage providers to upload directly to ReviewBoard. This includes defining the base class that can be subclassed to handle different file providers, and implementing webapi calls to allow users to interact with the back end. The webapi supports the following:

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

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

		Orignal: January 18, 2015

		Started working on the File-Provider webapi, model and objects.

		Added the file-provider api to the routes, and started working on pseudo-code on how the calls should work out. Started working on the model and objects for the fileprovider. Basic overview with how the object should work (with NotImplementedError for each function) as well as database schema for the model.

		UPDATE: January 23, 2015

		Updated the model and fileprovider.py to accomodate for third-party derived classes. Updated the webapi for GET (lists) and POST.

		UPDATE: January 30, 2015

		Updated the model to have two JSONfields, and added implementation for GET, POST, DELETE, and PUT requests and their test cases.
	+
	+	UPDATE: February 7, 2015
	+
	+	Made changes to the docstring for `FileProvider` objects and started working on them for `file_provider` webapi. Also, worked started on the child resource for FileProvider, `file_provider_content.py` which allows search, listing of contents and obtaining meta-information. Test cases were made for those as well. Added a few new errors for file providers, and worked on test objects `FileProvider`.

Testing Done:

~		Test cases made for list (GET, POST) and items (GET, PUT, DELETE)
	~	Test cases made for list (GET, POST) and items (GET, PUT, DELETE) for `file_provider` webapi
	+	Test cases made for list GET for `file_provider_content` webapi

Commit:

-	efa4cb93102614f0d5cabeba2746edd0f2deb963
+	8393330bec98f82b62846e869906bb74a62709e3

Diff:

Revision 6 (+1442 -1)

Show changes

	reviewboard/settings.py
	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/testing/fileprovider.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/errors.py
	8 more

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

reviewboard/settings.py (Diff revision 6)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 6)

Show all issues

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

reviewboard/settings.py (Diff revision 6)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 6)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/testing/fileprovider.py (Diff revision 6)
Show all issues
```
 'json_dump' imported but unused
```
reviewboard/testing/fileprovider.py (Diff revision 6)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/file_provider.py (Diff revision 6)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/file_provider_content.py (Diff revision 6)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/file_provider_content.py (Diff revision 6)
Show all issues
```
Col: 25
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/tests/test_file_provider.py (Diff revision 6)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/tests/test_file_provider.py (Diff revision 6)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6)
Show all issues
```
 'User' imported but unused
```
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6)
Show all issues
```
 'get_object_or_none' imported but unused
```
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6)
Show all issues
```
 'fp_content_item_mimetype' imported but unused
```
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6)
Show all issues
```
 'ExtraDataItemMixin' imported but unused
```
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Change Summary:

New commit for styling and PEP8/pyflakes

Commit:

-	8393330bec98f82b62846e869906bb74a62709e3
+	cb9218b72664ed6174192d6c2c40cfb8e4dbeec0

Diff:

Revision 7 (+1437 -1)

Show changes

	reviewboard/settings.py
	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/testing/fileprovider.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/errors.py
	8 more

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

reviewboard/settings.py (Diff revision 7)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 7)

Show all issues

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

reviewboard/settings.py (Diff revision 7)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/settings.py (Diff revision 7)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```

reviewboard/webapi/tests/test_file_provider.py (Diff revision 7)

Show all issues

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

VTL-Developer Feb. 7, 2015, 10:03 a.m.

This particular issue is now fixed on my end, would be in next commit.

reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
We use single quote strings.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
How about authenticate?
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
Likewise, how about deauthenticate?
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/fileproviders/models.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
This should be on the previous line.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```

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

Show all issues

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?

VTL-Developer Feb. 18, 2015, 12:23 p.m.

Changed the return model to FileProviderFile.

reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
No blank line here.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/testing/fileprovider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/webapi/resources/file_provider.py (Diff revision 7)
Show all issues
```
Since this is a JSON blob, this should have a dict type.
```
reviewboard/webapi/resources/file_provider.py (Diff revision 7)
Show all issues
```
Since this is a JSON field, this should have a dict type.
```
reviewboard/webapi/resources/file_provider.py (Diff revision 7)
Show all issues
```
Blank line between these.
```
reviewboard/webapi/resources/file_provider.py (Diff revision 7)
Show all issues
```
Wrap these in parenthesis to avoid using a backslash.
```

reviewboard/webapi/resources/file_provider.py (Diff revision 7)

Show all issues

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

data = {}

if credentials:
    data = json_load(credentials)

reviewboard/webapi/resources/file_provider.py (Diff revision 7)

Show all issues

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

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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.

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)
Show all issues
```
It should read like

`... or folder, e.g., ...
```
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)
Show all issues
```
This should be on the previous line.
```
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)
Show all issues
```
No blank line here.
```

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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?

VTL-Developer Feb. 9, 2015, 5:40 p.m.

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.

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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.

VTL-Developer Feb. 9, 2015, 5:43 p.m.

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

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)

Show all issues

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.

reviewboard/webapi/resources/file_provider_content.py (Diff revision 7)
Show all issues
```
Blank line between these.
```

reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 7)

Show all issues

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

reviewboard/webapi/tests/urls.py (Diff revision 7)
Show all issues
```
file_path should fit on the previous line.
```

Description:

		This FileProvider framework allows users to select files from their online file storage providers to upload directly to ReviewBoard. This includes defining the base class that can be subclassed to handle different file providers, and implementing webapi calls to allow users to interact with the back end. The webapi supports the following:

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

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

		Orignal: January 18, 2015

		Started working on the File-Provider webapi, model and objects.

		Added the file-provider api to the routes, and started working on pseudo-code on how the calls should work out. Started working on the model and objects for the fileprovider. Basic overview with how the object should work (with NotImplementedError for each function) as well as database schema for the model.

		UPDATE: January 23, 2015

		Updated the model and fileprovider.py to accomodate for third-party derived classes. Updated the webapi for GET (lists) and POST.

		UPDATE: January 30, 2015

		Updated the model to have two JSONfields, and added implementation for GET, POST, DELETE, and PUT requests and their test cases.

		UPDATE: February 7, 2015

		Made changes to the docstring for `FileProvider` objects and started working on them for `file_provider` webapi. Also, worked started on the child resource for FileProvider, `file_provider_content.py` which allows search, listing of contents and obtaining meta-information. Test cases were made for those as well. Added a few new errors for file providers, and worked on test objects `FileProvider`.
	+
	+	Update: February 11, 2015
	+
	+	Made changes that were suggested. Added documentation for the web api for both resources, and renamed the files.

Commit:

-	cb9218b72664ed6174192d6c2c40cfb8e4dbeec0
+	699beac810fa34019a658f6d7bc43807da344e22

Diff:

Revision 8 (+1456 -1)

Show changes

	reviewboard/settings.py
	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/testing/fileprovider.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/errors.py
	8 more

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

reviewboard/settings.py (Diff revision 8)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 8)

Show all issues

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

reviewboard/settings.py (Diff revision 8)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 8)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

reviewboard/webapi/resources/file_provider_file.py (Diff revision 8)

Show all issues

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

VTL-Developer Feb. 11, 2015, 1:15 p.m.

This and the next issue would be resolved in the next iteration. Either the comment block or the entire thing is removed.

reviewboard/webapi/resources/file_provider_file.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/file_provider_file.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
1. VT
  
  VTL-Developer Feb. 11, 2015, 1:15 p.m.
  Fixed on my end. Would be pushed in next iteration.
reviewboard/webapi/tests/test_file_provider.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/webapi/tests/test_file_provider_file.py (Diff revision 8)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 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.

chipx86 Feb. 11, 2015, 3:08 p.m.

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:

Fix up these errors.
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.
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.

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

Show all issues

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.

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

Show all issues

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.

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

Show all issues

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

reviewboard/fileproviders/fileprovider.py (Diff revision 8)
Show all issues
```
This should probably reference path now, right?
```

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

Show all issues

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

Same elsewhere.

reviewboard/fileproviders/fileprovider.py (Diff revision 8)
Show all issues
```
Must be one line.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 8)
Show all issues
```
Must be reworded to be one line.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 8)
Show all issues
```
Blank line between these.
```

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

Show all issues

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

reviewboard/fileproviders/fileprovider.py (Diff revision 8)
Show all issues
```
Why not just take *paths, rather than a leading argument?
```

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

Show all issues

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.

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

Show all issues

What does this do?

VTL-Developer Feb. 14, 2015, 8:33 a.m.

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.

reviewboard/fileproviders/fileprovider.py (Diff revision 8)
Show all issues
```
Global variables should be defined before all functions.
```

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

Show all issues

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

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

Show all issues

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?

reviewboard/fileproviders/models.py (Diff revision 8)
Show all issues
```
These must all be in alphabetical order.
```

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

Show all issues

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

reviewboard/fileproviders/models.py (Diff revision 8)
Show all issues
```
related_name= should probably go on the next line.
```
reviewboard/fileproviders/models.py (Diff revision 8)
Show all issues
```
This is only in Django 1.7, which we are not using.
```
reviewboard/fileproviders/models.py (Diff revision 8)
Show all issues
```
Both need to have null=True.
```

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

Show all issues

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?

reviewboard/fileproviders/models.py (Diff revision 8)
Show all issues
```
Missing a trailing period.
```

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

Show all issues

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

reviewboard/testing/fileprovider.py (Diff revision 8)
Show all issues
```
Isn't this the reason join_paths exists?
```

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

Show all issues

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

No blank line after.

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

Show all issues

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.

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

Show all issues

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

reviewboard/webapi/errors.py (Diff revision 8)
Show all issues
```
Missing a trailing period.
```

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

Show all issues

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

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

Show all issues

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

reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
Must be in alphabetical order.
```

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

Show all issues

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

reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
I don't see anything adding these errors?
```
reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
"require"
```

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

Show all issues

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

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

Show all issues

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.

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

Show all issues

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

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

Show all issues

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

reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
JSON"
```
reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
Should use isinstance(data, dict).
```

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

Show all issues

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

reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
This needs to list the field. See how it's done elsewhere.
```
reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
Same as above.
```

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

Show all issues

What's this?

VTL-Developer Feb. 14, 2015, 8:33 a.m.

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

VTL-Developer Feb. 21, 2015, 5:55 p.m.

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

reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
ame as above.
```

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

Show all issues

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.

reviewboard/webapi/resources/file_provider.py (Diff revision 8)
Show all issues
```
Same comment regarding where the spaces should go.
```
reviewboard/webapi/tests/mimetypes.py (Diff revision 8)
Show all issues
```
Needs to be file_provider_content_...
```

Change Summary:

Split the fileprovider webapi and related files into a new review request, also made changes to reflect suggestions.

Commit:

-	699beac810fa34019a658f6d7bc43807da344e22
+	529dd911a3525cd8d73f1a265cfa0d3d79fbb8d4

Diff:

Revision 9 (+308)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py

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

reviewboard/fileproviders/models.py (Diff revision 9)
Show all issues
```
Col: 14
 W291 trailing whitespace
```

Change Summary:

Updated the description, summary and testing done.

Summary:

-	[WIP] FileProvider implementation with Django
+	[WIP] FileProvider module implementation with Django

Description:

~		This FileProvider framework allows users to select files from their online file storage providers to upload directly to ReviewBoard. This includes defining the base class that can be subclassed to handle different file providers, and implementing webapi calls to allow users to interact with the back end. The webapi supports the following:
	~	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

~		Listing files at the specified path in your file storage
~		Searching for files in your file storage
	~	`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.
-		Uploading files from your file storage to ReviewBoard
-		Fetching a file's meta-data

~		As well as the general GET, POST, PUT and DELETE functionality.
	~	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
-
-		Orignal: January 18, 2015
-
-		Started working on the File-Provider webapi, model and objects.
-
-		Added the file-provider api to the routes, and started working on pseudo-code on how the calls should work out. Started working on the model and objects for the fileprovider. Basic overview with how the object should work (with NotImplementedError for each function) as well as database schema for the model.
-
-		UPDATE: January 23, 2015
-
-		Updated the model and fileprovider.py to accomodate for third-party derived classes. Updated the webapi for GET (lists) and POST.
-
-		UPDATE: January 30, 2015
-
-		Updated the model to have two JSONfields, and added implementation for GET, POST, DELETE, and PUT requests and their test cases.
-
-		UPDATE: February 7, 2015
-
-		Made changes to the docstring for `FileProvider` objects and started working on them for `file_provider` webapi. Also, worked started on the child resource for FileProvider, `file_provider_content.py` which allows search, listing of contents and obtaining meta-information. Test cases were made for those as well. Added a few new errors for file providers, and worked on test objects `FileProvider`.
-
-		Update: February 11, 2015
-
-		Made changes that were suggested. Added documentation for the web api for both resources, and renamed the files.

Testing Done:

~		Test cases made for list (GET, POST) and items (GET, PUT, DELETE) for `file_provider` webapi
	~	Test cases for functions and some object method in `reviewboard/fileproviders/tests.py`
-		Test cases made for list GET for `file_provider_content` webapi

Change Summary:

Added FileProviderFile.py, a model to represent the files found by FileProvider

Updated the documentation, and pep8 styling.

Added test cases for FileProviderFile.py

Commit:

-	529dd911a3525cd8d73f1a265cfa0d3d79fbb8d4
+	e2bc245565738d5c5cf41b672fa109355b22824d

Diff:

Revision 10 (+430)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/fileproviderfile.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py

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

reviewboard/fileproviders/fileproviderfile.py (Diff revision 10)
Show all issues
```
Col: 21
 E126 continuation line over-indented for hanging indent
```
reviewboard/fileproviders/fileproviderfile.py (Diff revision 10)
Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```

Change Summary:

Removed deauthenticate from the base FileProvider object. Updated the model to support unique constraints. Also, fixed the initialization of FileProviderFile to pass test cases.

Summary:

-	[WIP] FileProvider module implementation with Django
+	FileProvider module implementation with Django

Commit:

-	e2bc245565738d5c5cf41b672fa109355b22824d
+	90c11773cd81cedf318bb8f5a64588dd3fe77865

Diff:

Revision 11 (+425)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/fileproviderfile.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py

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

Many of my comments (particularly for the unit tests) apply in several places.

reviewboard/fileproviders/fileprovider.py (Diff revision 11)
Show all issues
```
Should use six.iterkeys(...)
```

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

Show all issues

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

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

Show all issues

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.

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

Show all issues

We should use constants for this.

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

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

Show all issues

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.

VTL-Developer Feb. 22, 2015, 6:49 p.m.

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.

VTL-Developer March 5, 2015, 3:40 p.m.

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

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

Show all issues

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

All these test-related comments apply throughout the file.

reviewboard/fileproviders/tests.py (Diff revision 11)
Show all issues
```
Blank line between these.
```

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

Show all issues

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.

reviewboard/fileproviders/tests.py (Diff revision 11)
Show all issues
```
You want self.assertRaises
```
reviewboard/fileproviders/tests.py (Diff revision 11)
Show all issues
```
Blank line between these.
```

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

Show all issues

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

Same below.

Change Summary:

Changed assertion as appropriate for the test cases. Removed error messages from assertions. Changed fileproviderfile.py to tree.py and separated the objects as necessary, and refactored any reference to the old FileProviderFile. Any pep8, other style changes and suggestions from reviews were also added.

Commit:

-	90c11773cd81cedf318bb8f5a64588dd3fe77865
+	8f023e1b4d81c91f15ed773f0e2ed013ff4e0fd4

Diff:

Revision 12 (+397)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

reviewboard/fileproviders/tests.py (Diff revision 12)
Show all issues
```
 local variable 'length' is assigned to but never used
```

Change Summary:

Removed unused variable

Commit:

-	8f023e1b4d81c91f15ed773f0e2ed013ff4e0fd4
+	d6ae24d2fdfdfb719a7b0cfdce7dd0be7b7bfae9

Diff:

Revision 13 (+396)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

Change Summary:

Refactored naming of variables:

path --> dir
file_path --> node_path

Updated the documentation and naming to reflect new refactored names

Commit:

-	d6ae24d2fdfdfb719a7b0cfdce7dd0be7b7bfae9
+	4423faf1b6ac4fe96115b111508460ed4439ae4e

Diff:

Revision 14 (+394)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

Change Summary:

Changed the initializer for Node to take in a full path, and split it accord

Changes to the test case to reflect these changes.

Commit:

-	4423faf1b6ac4fe96115b111508460ed4439ae4e
+	615484e058bc358f13950f80168729d2fd712651

Diff:

Revision 15 (+408)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

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.

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
These belong in the same import grouping.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
Blank line between these.
```

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

Show all issues

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

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

Show all issues

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.

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

Show all issues

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

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
"Return"
```

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

Show all issues

Instead of the modules listed as these, do:

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

etc.

Same below.

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
"specified path is not found"
```
reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
use :py:class: around ObjectDoesNotExist.
```
reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
"Return"
Same below.
```

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

Show all issues

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

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
The "Each file ..." should be its own paragraph.
```

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

Show all issues

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.

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
The "If no file ..." should be a new paragraph.
```

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

Show all issues

With path_sep going away, you can simplify this to:

"""Return a path composed from the given directory and file names."""

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

Show all issues

This can be one statement:

return '/%s' % '/'.join([
    ...
])

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
"Register"
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

"Register"

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

reviewboard/fileproviders/fileprovider.py (Diff revision 15)
Show all issues
```
"Unregister"
You can remove "from the list of known providers."
```

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

Show all issues

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

reviewboard/fileproviders/models.py (Diff revision 15)
Show all issues
```
Blank line betwen these.
```

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

Show all issues

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

reviewboard/fileproviders/models.py (Diff revision 15)
Show all issues
```
This must go last in the class.
```
reviewboard/fileproviders/models.py (Diff revision 15)
Show all issues
```
I'd maybe swap these.
```
reviewboard/fileproviders/models.py (Diff revision 15)
Show all issues
```
Needs a docstring.
```

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

Show all issues

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.

reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
No parens.
```

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

Show all issues

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

Also, how about FileProviderRegistrationTests?

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

Show all issues

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

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

Show all issues

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

reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
Instead of names_get, I'd just call this result.
```

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

Show all issues

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.

reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
This should be done in setUp.
```
reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
"with a"
Same below.
```

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

Show all issues

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.

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

Show all issues

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

Also, missing period.

reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
This should be done in setUp.
```
reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
Blank line between these.
```

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

Show all issues

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

reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
Should have a trailing comma.
Same below.
```
reviewboard/fileproviders/tests.py (Diff revision 15)
Show all issues
```
Blank line between these.
Same below.
```

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

Show all issues

"a Node."

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

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

Show all issues

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.

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

Show all issues

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

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

Show all issues

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.

reviewboard/fileproviders/tree.py (Diff revision 15)
Show all issues
```
Use single quotes when possible.
```

Change Summary:

Add changes as suggested from reviews.

- documentation clean-up
- changes to reviewboard.fileproviders.tree.Node initalization and variable 
- added normalize_path and changed join_path for FileProvider object
- add more documentation for some items
- test classes for FileProviderRegistrationTests turned into one class

Commit:

-	615484e058bc358f13950f80168729d2fd712651
+	ac51225ff5e9166f270aeb7545caa6128d03a8d4

Diff:

Revision 16 (+492)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

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

Show all issues

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

reviewboard/fileproviders/tests.py (Diff revision 16)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileproviders/tests.py (Diff revision 16)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileproviders/tests.py (Diff revision 16)
Show all issues
```
Col: 36
 E201 whitespace after '('
```
reviewboard/fileproviders/tests.py (Diff revision 16)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/fileproviders/tree.py (Diff revision 16)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Updated to fix for PEP-8 and Pyflakes styling.

Commit:

-	ac51225ff5e9166f270aeb7545caa6128d03a8d4
+	9d4c7dfe8c913ca2731ef62e12f0d1b7e70a7033

Diff:

Revision 17 (+496)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

Change Summary:

Updated description to include reviewboard/fileproviders/tree.py

Description:

		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.

		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

Change Summary:

Added new error NodeNotFound, to reflect changes made in /r/6948.

Fixed error in testing of FileProviderRegistrationTests, where list was used for hashing instead of name.

Updated documentation for reviewboard.fileproviders.fileprovider.FileProvider to reflect the new error and fixed mistakes.

Commit:

-	9d4c7dfe8c913ca2731ef62e12f0d1b7e70a7033
+	bc5a01c95cb73bbe1c6f8ff8e5c523b696963286

Diff:

Revision 18 (+502)

Show changes

	reviewboard/fileproviders/__init__.py
	reviewboard/fileproviders/errors.py
	reviewboard/fileproviders/fileprovider.py
	reviewboard/fileproviders/models.py
	reviewboard/fileproviders/tests.py
	reviewboard/fileproviders/tree.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



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

Description:

		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

reviewboard/fileproviders/fileprovider.py (Diff revision 18)
Show all issues
```
"with the file provider."
```
reviewboard/fileproviders/fileprovider.py (Diff revision 18)
Show all issues
```
I still think we should be removing this.
```

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

Show all issues

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.

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

Show all issues

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.

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

Show all issues

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.

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

Show all issues

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

reviewboard/fileproviders/tests.py (Diff revision 18)
Show all issues
```
"Unit tests"
```
reviewboard/fileproviders/tests.py (Diff revision 18)
Show all issues
```
This needs to call the parent tearDown().
```
reviewboard/fileproviders/tests.py (Diff revision 18)
Show all issues
```
"Unit tests"
```
reviewboard/fileproviders/tests.py (Diff revision 18)
Show all issues
```
This needs to call the parent setUp().
```
reviewboard/fileproviders/tests.py (Diff revision 18)

You have a pending review.

FileProvider module implementation with Django

Description:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Summary:

Description:

Commit:

Diff:

Testing Done:

Description:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Description:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Summary:

Description:

Testing Done:

Change Summary:

Commit:

Diff:

Change Summary:

Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Description:

Change Summary:

Commit:

Diff:

Description: