flake8
-
rbtools/clients/git.py (Diff revision 1) Show all issues
Review Request #11503 — Created March 4, 2021 and submitted
The way that the local repository type was detected overly complicated
and inefficient. We would go through and assemble a complete
RepositoryInfo
for each possible repository, potentially doing excess
computation that would just be thrown away. In addition, there were some
situtations where a repository would be operating in a remote-only mode
(for example, using SVN with the--repository-url
option) where we
were scanning everything locally before finally using the remote.This change reworks the local repository detection in RBTools to
work in three phases:
- First we ask each tool if it's operating in "remote only" mode. At
the moment, SVN is the only such tool, but it's easy to imagine other
server-heavy services gaining this ability.- Second, we scan through each tool to see if it thinks there's an
active repository. This has been simplified to only fetch the local
working directory from the tool, rather than the full repository
info.- Finally, after having chosen a tool, we ask it for the repository
info structure.
Summary | ID |
---|---|
a4ce2a41da89265ae7b025fd02327baeb3453186 |
Description | From | Last Updated |
---|---|---|
E501 line too long (83 > 79 characters) |
![]() |
|
Realizing it's kind of weird that we've been doing this based on path name length, and not number of directory … |
|
|
While here, can we change these to logging.warning? |
|
|
"Return ..." |
|
|
We can just return None at the end. Could even merge in the client_root check above with the client_root.lower() check, … |
|
|
We can skip the else: here, just return None. |
|
|
Since this is the Subversion code, we can probably make these docs more specific to Subversion's behavior. |
|
|
No need for parens. |
|
|
We can skip the else: and just return None. |
|
|
Totally your call here, but since we're almost certainly going to hvae a result from this call, if info is … |
|
|
Is this here so that Git and Mercurial can get access to it? Given how similar the name is to … |
|
|
Can you add a blank line between these? Any reason we can't import this at the top of the file? |
|
|
No need for the else:. |
|
|
We have a few versions of the docstring for this function. Let's copy/paste one summary for all of these, at … |
|
|
No need for the else:. |
|
|
No need for the else:. |
|
|
No need for the else:. |
|
|
No need for the else:. |
|
|
Why the direct comparison? Is None a valid option? |
|
|
Can you add Version Added? |
|
|
Can you add Version Added? |
|
|
There's a danger with this line (and any like it). One of the warts of Python. >>> d = {} … |
|
|
This could be: local_path = info.get('Working Copy Root Path') |
|
|
Can this use the new deprecated class? |
|
|
These still have to go before Returns, in order to render correctly. |
|
|
Same here. |
|
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+848 -442) |
Design looks great. Most of my comments are about consistency in docs and some logic flows.
rbtools/clients/__init__.py (Diff revision 2) |
---|
Realizing it's kind of weird that we've been doing this based on path name length, and not number of directory components. Should we revisit that?
Right now, this:
/src/oh-boy-this-repository-name-is-long`
is considered deeper than:
/src/repos/libfoo/src
rbtools/clients/perforce.py (Diff revision 2) |
---|
We can just
return None
at the end.Could even merge in the
client_root
check above with theclient_root.lower()
check, and simplify it all further.
rbtools/clients/svn.py (Diff revision 2) |
---|
Since this is the Subversion code, we can probably make these docs more specific to Subversion's behavior.
rbtools/clients/svn.py (Diff revision 2) |
---|
Totally your call here, but since we're almost certainly going to hvae a result from this call, if
info
is set, we can skip the extra checks and do:try: path = info['Repository Root'] ... except KeyError: return None
rbtools/clients/svn.py (Diff revision 2) |
---|
Is this here so that Git and Mercurial can get access to it?
Given how similar the name is to the
SCMClient
version, maybe we should rename it. I was pretty confused for a second.
rbtools/clients/tests/test_scanning.py (Diff revision 2) |
---|
Can you add a blank line between these?
Any reason we can't import this at the top of the file?
rbtools/clients/tfs.py (Diff revision 2) |
---|
We have a few versions of the docstring for this function. Let's copy/paste one summary for all of these, at least, so they don't diverge further down the road.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+864 -502) |
Fix remote-only mode with SVN.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+870 -500) |
rbtools/clients/svn.py (Diff revision 4) |
---|
There's a danger with this line (and any like it). One of the warts of Python.
>>> d = {} >>> print(d and 'key' in d) {} >>> n = None >>> print(n and 'key' in n) None
Due to the old-style ternary-ish statements Python allowed, we have to be very careful to explicitly compare each part of this if we're going to return it as a boolean. So,
info is not None
orbool(info)
or whatever makes sense here.I know we've done this wrong in places, and while it normally isn't an issue, I have fixed an obscure bug or two in the past due to this sort of quirk.
This might apply to other parts of the code. I haven't gone through carefully, just noticed this here.
rbtools/clients/svn.py (Diff revision 4) |
---|
This could be:
local_path = info.get('Working Copy Root Path')
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+876 -500) |
rbtools/clients/__init__.py (Diff revision 5) |
---|
These still have to go before
Returns
, in order to render correctly.