flake8
-
rbtools/clients/__init__.py (Diff revision 1) Show all issues -
-
-
-
rbtools/clients/clearcase.py (Diff revision 1) F841 local variable 'e' is assigned to but never used
-
-
-
-
-
-
-
-
-
-
-
-
-
Review Request #11536 — Created March 21, 2021 and submitted
Information | |
---|---|
david | |
RBTools | |
master | |
Reviewers | |
rbtools | |
This is a major rework of the remote repository detection mechanism
within RBTools. The old implementation had a lot of issues, worst of
which was that there was a lot of inconsistency across different tool
implementations (for example,rbt post
had a lot of additional
detection code that wasn't shared with other tools).In the new implementation, we first try to fetch remote repositories,
filtering by the configured repository name, or detected repository
path(s). If this returns a single result, we can immediately return
that. If that fails, it means we're in a situation where the configured
paths on the server are different from the local path (including any
mirror_path), and we need to match in a different way. Individual tools
may implementfind_matching_server_repository
in order to do
tool-specific comparisons (which right now is done by SVN with
repository UUID and ClearCase with VOB UUID). This new method replaces
the oldRepositoryInfo.find_server_repository_info
method, which
always felt super ugly and clunky. In lieu of that,RepositoryInfo
subclasses can implementupdate_from_remote
, which allows them to
include information from the remote repository or repository info
resource.Commands which want to make use of the repository can now set the
needs_repository
attribute. In this case, the command initialization
will handle finding the remote repository and updating the
RepositoryInfo
appropriately. This corrects a couple issues where
commands were using the repository but forgetting to call the old
find_server_repository_info
, potentially ending up with incorrect
data. Thesetup-repo
command works a little differently because it
needs to potentially do that multiple times, but it uses the same
underlying mechanism to attempt to pre-filter the list of matching
repositories in a useful way.This includes several other improvements and cleanups along the way:
rbt setup-repo
's server initialization has been fixed to merge--server
and interactive entry. This both improves thetool=
for APIrbt land
: Tested basic behavior.rbt patch
: Tested basic behavior.rbt post
: Posted new and updated changes against Git, including with-u
. Created a remote SVN repository and posted purely remote commits--repository-url
.rbt setup-repo
: Tested with both --server
and specifying therbt stamp
: Tested basic behavior.rbt status
: tested both with and without --all
. Verified thatSummary | |
---|---|
Description | From | Last Updated |
---|---|---|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
F841 local variable 'e' is assigned to but never used |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (166 > 79 characters) |
![]() |
|
E501 line too long (170 > 79 characters) |
![]() |
|
E501 line too long (161 > 79 characters) |
![]() |
|
E501 line too long (157 > 79 characters) |
![]() |
|
E501 line too long (127 > 79 characters) |
![]() |
|
E501 line too long (118 > 79 characters) |
![]() |
|
E501 line too long (145 > 79 characters) |
![]() |
|
E501 line too long (146 > 79 characters) |
![]() |
|
E501 line too long (153 > 79 characters) |
![]() |
|
E501 line too long (146 > 79 characters) |
![]() |
|
E501 line too long (111 > 79 characters) |
![]() |
|
Can you add a Version Added and Type (in that order)? |
|
|
Can we make this a list of strings instead? I feel like this would be a bit nicer. |
|
|
Can you add Version Added? |
|
|
Can you add Version Added? |
|
|
Can you add Deprecated in here? |
|
|
Can we move these private methods after the public ones? |
|
|
This needs to be a full class path. |
|
|
No blank line here. |
|
|
This also needs to be a full class path. |
|
|
Can you add Deprecated here, and any other methods that emit a deprecation warning? |
|
|
The pattern I've been moving to is to import kgb directly, so we don't have to bother updating the import … |
|
|
E501 line too long (166 > 79 characters) |
![]() |
|
Review Bot is going to complain about this forever. We could maybe do: ('http://...... '......'): { .... }, ... Also, … |
|
|
E501 line too long (170 > 79 characters) |
![]() |
|
E501 line too long (161 > 79 characters) |
![]() |
|
E501 line too long (157 > 79 characters) |
![]() |
|
E501 line too long (127 > 79 characters) |
![]() |
|
E501 line too long (118 > 79 characters) |
![]() |
|
While here, want to move to @self.spy_for(urlopen)? |
|
|
Can we move this to the top? Or is this leftover debugging? |
|
|
Can you add Version Added and Type (in that order)? |
|
|
Can these be keyword arguments? |
|
|
The description shouldn't be indented. |
|
|
Can you add a Version Added? |
|
|
Missing a trailing comma. |
|
|
Alternatively: query.pop('path', None) |
|
|
To render correctly, this needs to be above Args, so it fits with the doc body. |
|
|
I like the , deprecated. We haven't done that before, but I think that's a great tag. We should put … |
|
|
A bunch of args (these onward) are documented as non-optional, but have a preset value. I assume this is because … |
|
|
This will also need to move above Args. |
|
|
For new code, let's do import kgb and then reference kgb.SpyAgency. This makes it easy to later use kgb.SpyOpReturn or … |
|
|
No need for parens here, or in the ones below. |
|
|
E501 line too long (145 > 79 characters) |
![]() |
|
E501 line too long (146 > 79 characters) |
![]() |
|
E501 line too long (153 > 79 characters) |
![]() |
|
E501 line too long (146 > 79 characters) |
![]() |
|
E501 line too long (111 > 79 characters) |
![]() |
|
Let's do: @self.spy_for(urlopen) |
|
|
Shouldn't these all say "Testing get_repository_resource"? |
|
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
Before "Args". "Args" and "Returns" are special, and must come after. |
|
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
This needs to move before Returns, since it'll be part of the description text. |
|
|
Same here. |
|
|
Can you update this to use the full class path? |
|
|
Missing space before "In". |
|
|
Looking at the latest revision, do we still have any reason to default api_client and api_root to None? |
|
rbtools/clients/__init__.py (Diff revision 1) |
---|
rbtools/clients/clearcase.py (Diff revision 1) |
---|
F841 local variable 'e' is assigned to but never used
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+1874 -776) |
David, this is great. It really came together.
My comments are almost entirely doc-related, with a couple small suggestions here and there. The overall architecture, what an improvement.
rbtools/clients/__init__.py (Diff revision 2) |
---|
Can we make this a list of strings instead? I feel like this would be a bit nicer.
rbtools/clients/clearcase.py (Diff revision 2) |
---|
Can we move these private methods after the public ones?
rbtools/clients/clearcase.py (Diff revision 2) |
---|
Can you add
Deprecated
here, and any other methods that emit a deprecation warning?
rbtools/clients/tests/test_svn.py (Diff revision 2) |
---|
The pattern I've been moving to is to
import kgb
directly, so we don't have to bother updating the import list to use any specific kgb tooling.
rbtools/clients/tests/test_svn.py (Diff revision 2) |
---|
Review Bot is going to complain about this forever. We could maybe do:
('http://...... '......'): { .... }, ...
Also, maybe take the base URL out of these and put them into a variable to both shorten these and make that part more contextually apparent?
rbtools/clients/tests/test_svn.py (Diff revision 2) |
---|
While here, want to move to
@self.spy_for(urlopen)
?
rbtools/clients/tests/test_svn.py (Diff revision 2) |
---|
Can we move this to the top? Or is this leftover debugging?
rbtools/utils/review_request.py (Diff revision 2) |
---|
To render correctly, this needs to be above
Args
, so it fits with the doc body.
rbtools/utils/review_request.py (Diff revision 2) |
---|
I like the
, deprecated
. We haven't done that before, but I think that's a great tag. We should put that in Notion.
rbtools/utils/review_request.py (Diff revision 2) |
---|
A bunch of args (these onward) are documented as non-optional, but have a preset value. I assume this is because of the deprecated parameters preceding them. We should make sure to enforce any that are indeed required through checks at the start of the function.
rbtools/utils/tests/test_repository.py (Diff revision 2) |
---|
For new code, let's do
import kgb
and then referencekgb.SpyAgency
. This makes it easy to later usekgb.SpyOpReturn
or other methods without extending the import list.
rbtools/utils/tests/test_repository.py (Diff revision 2) |
---|
No need for parens here, or in the ones below.
rbtools/utils/tests/test_repository.py (Diff revision 2) |
---|
Shouldn't these all say "Testing get_repository_resource"?
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+2358 -1112) |
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
rbtools/utils/tests/test_repository.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+2358 -1112) |
rbtools/clients/tests/test_svn.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
rbtools/clients/tests/test_svn.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
rbtools/clients/tests/test_svn.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
rbtools/clients/tests/test_svn.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
rbtools/clients/tests/test_svn.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
rbtools/clients/tests/test_svn.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+2358 -1112) |
rbtools/clients/__init__.py (Diff revision 5) |
---|
This needs to move before
Returns
, since it'll be part of the description text.
rbtools/utils/review_request.py (Diff revision 5) |
---|
Looking at the latest revision, do we still have any reason to default
api_client
andapi_root
toNone
?
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+2336 -1090) |
rbtools/clients/__init__.py (Diff revisions 4 - 6) |
---|
Before "Args". "Args" and "Returns" are special, and must come after.