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

Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 2 (+310) |

-
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) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 2) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/resources/file_provider.py (Diff revision 2) 'webapi_response_errors' imported but unused
-
reviewboard/webapi/resources/file_provider.py (Diff revision 2) Col: 5 E265 block comment should start with '# '
-
reviewboard/webapi/resources/file_provider.py (Diff revision 2) Col: 5 E303 too many blank lines (2)
-
reviewboard/webapi/resources/file_provider.py (Diff revision 2) Col: 13 E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+310) |

-
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) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 3) Col: 1 E302 expected 2 blank lines, found 1
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+312) |

-
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) 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 seename
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? -
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4) 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) 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) 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."
-
-
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4) The
"""
should be on the first line when there's just a summary. -
reviewboard/fileprovidersvcs/fileprovider.py (Diff revision 4) 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) When possible, use single quotes instead of double quotes.
We should also rename this function, since
list
is a reserved word. -
reviewboard/fileprovidersvcs/models.py (Diff revision 4) 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) I know older code does this, but newer code can use
@cached_property
and not have to worry about thehasattr
stuff. See Django's docs on that.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+847 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/root.py reviewboard/webapi/errors.py reviewboard/testing/fileprovider.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/root.py reviewboard/webapi/errors.py reviewboard/testing/fileprovider.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 5) Col: 1 E302 expected 2 blank lines, found 1
-
-
-
reviewboard/settings.py (Diff revision 5) 'from settings_local import *' used; unable to detect undefined names
-
-
-
-
-
reviewboard/webapi/resources/file_provider.py (Diff revision 5) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 5) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 5) Col: 62 E231 missing whitespace after ','
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 5) 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
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+1442 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/file_provider_content.py reviewboard/webapi/errors.py reviewboard/testing/fileprovider.py reviewboard/webapi/tests/test_file_provider_content.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/file_provider_content.py reviewboard/webapi/errors.py reviewboard/testing/fileprovider.py reviewboard/webapi/tests/test_file_provider_content.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py
-
-
reviewboard/settings.py (Diff revision 6) 'from settings_local import *' used; unable to detect undefined names
-
-
-
-
-
reviewboard/webapi/resources/file_provider.py (Diff revision 6) Col: 5 E303 too many blank lines (2)
-
reviewboard/webapi/resources/file_provider_content.py (Diff revision 6) Col: 5 E303 too many blank lines (2)
-
reviewboard/webapi/resources/file_provider_content.py (Diff revision 6) Col: 25 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 6) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 6) Col: 80 E501 line too long (89 > 79 characters)
-
-
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6) 'get_object_or_none' imported but unused
-
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6) 'fp_content_item_mimetype' imported but unused
-
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6) 'ExtraDataItemMixin' imported but unused
-
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 6) Col: 80 E501 line too long (89 > 79 characters)
Change Summary:
New commit for styling and PEP8/pyflakes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+1437 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/file_provider_content.py reviewboard/webapi/errors.py reviewboard/testing/fileprovider.py reviewboard/webapi/tests/test_file_provider_content.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/file_provider_content.py reviewboard/webapi/errors.py reviewboard/testing/fileprovider.py reviewboard/webapi/tests/test_file_provider_content.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py
-
-
reviewboard/settings.py (Diff revision 7) 'from settings_local import *' used; unable to detect undefined names
-
-
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 7) Col: 80 E501 line too long (81 > 79 characters)
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
reviewboard/testing/fileprovider.py (Diff revision 7) There should probably be a more specific exception for this (and elsewhere you're raising
KeyError
). Perhaps something along the lines of a file not found error.Also, if the file doesn't have metadata, shouldn't it return
None
or an empty dictionary instead? -
-
-
-
-
reviewboard/webapi/resources/file_provider.py (Diff revision 7) Since this is a JSON blob, this should have a
dict
type. -
reviewboard/webapi/resources/file_provider.py (Diff revision 7) Since this is a JSON field, this should have a
dict
type. -
-
reviewboard/webapi/resources/file_provider.py (Diff revision 7) Wrap these in parenthesis to avoid using a backslash.
-
reviewboard/webapi/resources/file_provider.py (Diff revision 7) We don't use the Python ternary. Instead use a regular
if-else
ordata = {} if credentials: data = json_load(credentials)
-
reviewboard/webapi/resources/file_provider.py (Diff revision 7) 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) 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) 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) Use double backticks for things marked as code in restructured text. Here and elsewhere.
-
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7) It should read like
`... or folder, e.g., ...
-
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7) This should be on the previous line.
-
-
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7) Are we safe to assume that all instances of
FileProvider
will use the forward slash? Should there be something akin toos.path.join
andos.path.sep
on eachFileProvider
instance? -
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7) 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. -
reviewboard/webapi/resources/file_provider_content.py (Diff revision 7) 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) 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 theexc_info=1
parameter and it will automatically log the exception information. -
-
reviewboard/webapi/tests/test_file_provider_content.py (Diff revision 7) All test methods should have a docstring describing what they are testing. Here and elsewhere.
-
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+1456 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/root.py reviewboard/webapi/errors.py reviewboard/webapi/tests/test_file_provider_file.py reviewboard/testing/fileprovider.py reviewboard/webapi/resources/file_provider_file.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/__init__.py reviewboard/settings.py reviewboard/webapi/resources/file_provider.py reviewboard/fileproviders/models.py reviewboard/webapi/resources/root.py reviewboard/webapi/errors.py reviewboard/webapi/tests/test_file_provider_file.py reviewboard/testing/fileprovider.py reviewboard/webapi/resources/file_provider_file.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/webapi/tests/test_file_provider.py reviewboard/fileproviders/fileprovider.py Ignored Files: reviewboard/fileproviders/__init__.py reviewboard/fileproviders/tests.py
-
-
reviewboard/settings.py (Diff revision 8) 'from settings_local import *' used; unable to detect undefined names
-
-
-
reviewboard/webapi/resources/file_provider_file.py (Diff revision 8) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/resources/file_provider_file.py (Diff revision 8) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/resources/file_provider_file.py (Diff revision 8) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_file_provider.py (Diff revision 8) Col: 80 E501 line too long (81 > 79 characters)
-
reviewboard/webapi/tests/test_file_provider_file.py (Diff revision 8) 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.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 8) 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) 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) Probably just
filename
for consistency with the rest of the codebase. -
reviewboard/fileproviders/fileprovider.py (Diff revision 8) This should probably reference
path
now, right? -
reviewboard/fileproviders/fileprovider.py (Diff revision 8) Let's do
path
instead offile_path
, for consistency.Same elsewhere.
-
-
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 8) I have a number of comments about this function. Given those comments, I think this can really just be:
return self.path_sep.join([ path.strip(self.path_sep) for path in paths ]) or self.path_sep
-
reviewboard/fileproviders/fileprovider.py (Diff revision 8) Why not just take
*paths
, rather than a leading argument? -
reviewboard/fileproviders/fileprovider.py (Diff revision 8) Blank line between these.
This looks like it's protecting the caller from itself. It should be considered invalid to pass non-strings or strings with invalid characters (like whitespace), rather than working around it. We could be hiding potential problems from the caller until later this way.
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 8) Global variables should be defined before all functions.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 8) Must be a single line.
Same elsewhere. I'll let you look at the rest :)
-
reviewboard/fileproviders/fileprovider.py (Diff revision 8) These should be able to start on the
logging.error()
line.You can also pass in
entry
ande
as parameters tologging.error()
.Can you also pass
exc_info=1
tologging.error()
, so that we can get detailed information when something goes wrong? -
-
reviewboard/fileproviders/models.py (Diff revision 8) This information is obvious from looking at the class. Can you instead alter the documentation to describe the point of the class's existence?
-
reviewboard/fileproviders/models.py (Diff revision 8) related_name=
should probably go on the next line. -
reviewboard/fileproviders/models.py (Diff revision 8) This is only in Django 1.7, which we are not using.
-
-
reviewboard/fileproviders/models.py (Diff revision 8) We don't use the
if ... else ...
form in the codebase (outside of lambdas), as it's less explicit and easier to misread. Can you change this to an explicit set of if/else blocks? -
-
reviewboard/testing/fileprovider.py (Diff revision 8) For
sys
modules, it's best to just import the top-levelsys
and access functions likesys.getsizeof
. -
-
reviewboard/testing/testcase.py (Diff revision 8) The
"""
must be on the same line as the summary, in this case.No blank line after.
-
reviewboard/testing/testcase.py (Diff revision 8) This should be a single
return FileProviderAccount.objects.create(...)
statement.Also, when listing multiple lines of parameters in keyword form, you should have only one parameter per line.
-
reviewboard/webapi/errors.py (Diff revision 8) Maybe start this off at 250, to give more room for repository-related errors.
-
-
-
reviewboard/webapi/resources/file_provider.py (Diff revision 8) This should just import
json
and usejson.loads
, which will be easier to grep and read. -
-
reviewboard/webapi/resources/file_provider.py (Diff revision 8) Many of these have trailing commas that shouldn't be added.
-
reviewboard/webapi/resources/file_provider.py (Diff revision 8) I don't see anything adding these errors?
-
-
reviewboard/webapi/resources/file_provider.py (Diff revision 8) Feels like "... in order to allow access ..." might be easier to read.
-
reviewboard/webapi/resources/file_provider.py (Diff revision 8) 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) We should have
is_mutable_by
functions onFileProviderAccount
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) 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) 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) This needs to list the field. See how it's done elsewhere.
-
-
-
-
reviewboard/webapi/resources/file_provider.py (Diff revision 8) This loses out on things like count queries and pagination. Instead,
get_queryset()
should be returningrequest.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) Same comment regarding where the spaces should go.
-
Change Summary:
Split the fileprovider webapi and related files into a new review request, also made changes to reflect suggestions.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+308) |

-
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
-
Change Summary:
Updated the description, summary and testing done.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+430) |

-
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) Col: 21 E126 continuation line over-indented for hanging indent
-
reviewboard/fileproviders/fileproviderfile.py (Diff revision 10) 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 ofFileProviderFile
to pass test cases.
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 11 (+425) |

-
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) 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) The file this is in should probably be called
file.py
, sincefileproviders/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) 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) 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.
-
reviewboard/fileproviders/tests.py (Diff revision 11) Test docstrings must start with "Testing" instead of "Test".
All these test-related comments apply throughout the file.
-
-
reviewboard/fileproviders/tests.py (Diff revision 11) You can use assertIn`.
We don't provide error messages in the assertions. They make the code a bit too verbose and aren't really that useful in practice. I'd remove them from all tests.
-
-
-
reviewboard/fileproviders/tests.py (Diff revision 11) True
doesn't tell me anything. In cases like this, it's best to use keyword arguments.Same below.
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+397) |

-
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) local variable 'length' is assigned to but never used
Change Summary:
Removed unused variable
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+396) |

-
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+394) |

-
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+408) |

-
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) These belong in the same import grouping.
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) Can you add doc comments for each of these? They're in the form of:
#: Single-line summary. varname = ... #: Single-line summary. #: #: Multi-line #: description. varname = ...
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) I think it's reasonable to standardize on '/' as a path separator everywhere, and to just assume that '/' is the root. No need to define variables. It'll just make things more complicated later.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) 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) Instead of the modules listed as these, do:
:py:class:`reviewboard.fileprovider.tree.File`
etc.
Same below.
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) use
:py:class:
aroundObjectDoesNotExist
. -
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) 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) The "Each file ..." should be its own paragraph.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) I don't know that we need to bother with the whole part about the input stream. It's already clear that a Django File instance must be returned, with all the requirements that come with that.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) The "If no file ..." should be a new paragraph.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) With
path_sep
going away, you can simplify this to:"""Return a path composed from the given directory and file names."""
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) This can be one statement:
return '/%s' % '/'.join([ ... ])
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) You can use
logging.exception
, and you won't have to passexc_info=1
. -
reviewboard/fileproviders/fileprovider.py (Diff revision 15) 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) This says it's returning the classes, but the function name says it's the names.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) viewkeys
is only in Python 2.7. This should usesix.iterkeys
instead. -
reviewboard/fileproviders/fileprovider.py (Diff revision 15) "Register"
This isn't necessarily a third party's class. Just say "Register a file provider class."
-
reviewboard/fileproviders/fileprovider.py (Diff revision 15) "Unregister"
You can remove "from the list of known providers."
-
reviewboard/fileproviders/models.py (Diff revision 15) This model is meant to be consumed, and shouldn't know how it'll be consumed. Today, it's the API. Tomorrow, other things might use this. We should avoid saying how a model will be used, but rather what it's there for (storing information needed to authenticate and communicate with an online file provider).
-
-
reviewboard/fileproviders/models.py (Diff revision 15) For various reasons, having a
JSONField
withnull=True
will cause problems. Instead, usedefault={}
. -
-
-
-
reviewboard/fileproviders/models.py (Diff revision 15) These should say "Return ..." and be fleshed out to describe the conditions upon which they're accessible/mutable. See the other equivalent functions in other classs for examples.
-
-
reviewboard/fileproviders/tests.py (Diff revision 15) This should probably have a
tearDown
that resets the registered file providers.Also, how about
FileProviderRegistrationTests
? -
reviewboard/fileproviders/tests.py (Diff revision 15) Maybe just have one of these outside of the TestCase, since all the tests are repeating the same pattern.
-
reviewboard/fileproviders/tests.py (Diff revision 15) That's kinda weird to assign
object
to this. How about justNone
? -
reviewboard/fileproviders/tests.py (Diff revision 15) Instead of
names_get
, I'd just call thisresult
. -
reviewboard/fileproviders/tests.py (Diff revision 15) These two things don't match.
This testcase can easily be used for more than the join_path function. The docstring should just say it's testing FileProvider functions.
-
-
-
-
reviewboard/fileproviders/tests.py (Diff revision 15) I'd argue that a blank string shouldn't be treated as a valid component of a path.
os.path.join
will skip any blank strings. -
reviewboard/fileproviders/tests.py (Diff revision 15) This should be more than just initialization. Just keep it as "Unit tests for FileProviderFile."
Also, missing period.
-
-
-
-
reviewboard/fileproviders/tests.py (Diff revision 15) This is about FileProviderDir, but is in the FileProviderFile tests.
-
-
-
reviewboard/fileproviders/tree.py (Diff revision 15) "a Node."
This shouldn't have any idea of how it's going to be subclassed and used.
-
reviewboard/fileproviders/tree.py (Diff revision 15) This is pretty thick and verbose.
I'd have a paragraph per parameter.
Also, some other comments:
file_provider
should be the first parameter (pretty common to have the owner of an object be the first, in these situations).path
would be better thannode_path
.meta_information
is better asmetadata
.
-
reviewboard/fileproviders/tree.py (Diff revision 15) We do the strip a lot. How about instead, you add a
normalize_path
function toFileProvider
. -
reviewboard/fileproviders/tree.py (Diff revision 15) All paths should be required to be absolute (start with
'/'
).Once that condition can be guaranteed, you don't need this
if
statement. You can just do thersplit
. -
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+492) |

-
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) Col: 13 E128 continuation line under-indented for visual indent
-
-
-
-
-
reviewboard/fileproviders/tree.py (Diff revision 16) Col: 80 E501 line too long (80 > 79 characters)
Change Summary:
Updated to fix for PEP-8 and Pyflakes styling.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+496) |

-
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: |
|
---|
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 18 (+502) |

-
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: |
|
---|
-
-
-
reviewboard/fileproviders/fileprovider.py (Diff revision 18) I still think we should be removing this.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 18) This all feels like it's getting complicated again, trying to adjust for bad programming. We don't need to handle users typing in paths. We really should just be requiring code to be forming valid paths to begin with.
-
reviewboard/fileproviders/fileprovider.py (Diff revision 18) I think this is all just too complicated as well. I know this came up in Slack at one point, but I really believe that it's the caller's fault if they're providing a path with, say, nothing between delimiters, or with
..
, or whatever. Just like a URL, the path needs to be correct to start with. That's got to be the responsibility of the caller. -
reviewboard/fileproviders/fileprovider.py (Diff revision 18) Same. This really should just strip. It's not about protecting the caller here either, but rather about having a normalized path ready to join new path items to.
-
reviewboard/fileproviders/models.py (Diff revision 18) These calls will always perform a database query, even if we don't need it. It's best to avoid this.
We can do that by checking
self.user_id
againstuser.pk
. That means checkinguser.is_authenticated()
first, though, like:return user.is_authenticated() and self.user_id == user.pk
-
-
-
-