Make RBTools command initialization easier and more consistent.
Review Request #11480 — Created Feb. 22, 2021 and submitted
The different commands in RBTools have lots of differerent needs. Some
of them interact only with your local repository, some of them interact
only with the server API, some do both, and some do neither. We've
traditionally had a few methods to handle things like setting up the API
or the SCMClient, but usage of these was inconsistent and resulted in a
lot of repeated boilerplate. Because of this and the history of RBTools
development, usage of all of this was somewhat inconsistent, with many
commands doing more or less than they really needed to do.This change is the first of a couple to clean this up. Two new
attributes have been added to the baseCommand
class:needs_api
and
needs_scm_client
. Subclasses can set these toTrue
depending on
their needs, and then prior tomain
being called, the initialization
process will set various attributes:api_client
,api_root
,
capabilities
,repository_info
,server_url
, andtool
.This change handles most of the initialization process. The one major
remaining piece are the calls tofind_server_repository_info
, which
will require some more involved changes (for example, short-cutting in
the case whenREPOSITORY_NAME
orREPOSITORY_URL
are defined).
- Smoke tested basic functionality for all changed commands.
- Verified
rbt patch
behavior was correct with both ID and full URL
(ID only and ID + diff revision) arguments. Checked that
rbt patch --print
did not initialize any repository support. - Verified
rbt land
behavior was correct with both ID and full URL
--review-request-id
option. - Checked that
rbt status --all
did not initialize any repository
support.
Summary | ID |
---|---|
7a393157dfa946d52898718a1e5d0a969df39591 |
Description | From | Last Updated |
---|---|---|
Let's move this work to master for 3.0, given how much of a change I suspect this will all ultimately … |
chipx86 | |
F821 undefined name 'tool' |
reviewbot | |
F821 undefined name 'repository_info' |
reviewbot | |
F821 undefined name 'repository_info' |
reviewbot | |
F821 undefined name 'repository_info' |
reviewbot | |
F821 undefined name 'repository_info' |
reviewbot | |
F821 undefined name 'repository_info' |
reviewbot | |
Typo: "repat" |
chipx86 | |
Private methods should go below public. |
chipx86 | |
"command line" |
chipx86 | |
Should we just assign server_url unconditionally? |
chipx86 | |
This should be one if statement. It's checking server_url is None twice, and there's never an alternate condition for the … |
chipx86 | |
This doesn't handle the case of trees[path] not existing. |
chipx86 | |
This should be logging.warning. The warn alias is, as far as I can tell, for backwards-compatibility only. Actually though, I … |
chipx86 | |
This should also be updated to use warnings and ideally a version-specific DeprecationWarning. |
chipx86 | |
Can we pull out self.tool into a local variable to avoid the repeated attribute lookups and line changes? |
chipx86 | |
Missing a Raises |
chipx86 | |
While here, can we make these keyword arguments? |
chipx86 | |
Let's pull out api_client to avoid repeated attribute lookups. |
chipx86 | |
Missing a Raises:. |
chipx86 | |
Let's pull out tool as well. |
chipx86 | |
Let's pull out repository_info. |
chipx86 | |
While here, can we make these all keyword arguments? This is a really wordy function. |
chipx86 | |
Can we use keyword arguments here? |
chipx86 | |
And here. |
chipx86 | |
Can we use keyword arguments here? |
chipx86 | |
How about we call this initialize? That'll be consistent with the naming used for extensions. |
chipx86 | |
Does this fit on one line now? |
chipx86 | |
Can you add a Version Added to these (before Type)? |
chipx86 |
- Commits:
-
Summary ID 3bc91c87ed92af992896bf4bd7959a306bf91e4b ba7733ecfbeca09c3138676462524ed6c1da1b45
Checks run (2 succeeded)
-
This is a great cleanup! This cleanly removes one of the biggest annoyances with writing commands. I love it.
My biggest nit is that I'd like to move toward fewer, rather than more, attribute accesses. We have a lot of code (existing and newly-introduced in this change) that does a lot of repeated
self.some_attr_name
, and while it's not performance-critical code, it's slower and less readable to keep doing that. I suspect that this change will shrink a bit if we replace some of the older code you're getting rid of with statements liketool = self.tool
when that variable's going to be used repeatedly.I marked a few cases like this, but it's not comprehensive.
There's a few other areas of code that I've always disliked, where we're calling some utility function with a bunch of positional arguments, and I think there's a good opportunity to switch those calls to keyword arguments. I only marked up a few of the bigger cases here.
-
Let's move this work to
master
for 3.0, given how much of a change I suspect this will all ultimately be. -
-
-
-
-
This should be one
if
statement. It's checkingserver_url is None
twice, and there's never an alternate condition for the outerif
. -
-
This should be
logging.warning
. Thewarn
alias is, as far as I can tell, for backwards-compatibility only.Actually though, I think we want to use the
warnings
module. Ideally, and this can be a follow-up change if you want, we should have arbtools.deprecation
module that provides classes similar to Review Board's, so we can put together a proper deprecation strategy. -
-
Can we pull out
self.tool
into a local variable to avoid the repeated attribute lookups and line changes? -
-
-
-
-
-
-
-
-
-
- Commits:
-
Summary ID ba7733ecfbeca09c3138676462524ed6c1da1b45 272daa73034353600c3447ed408c58c36cc8a251 - Branch:
-
release-2.0.xmaster
Checks run (2 succeeded)
- Commits:
-
Summary ID 272daa73034353600c3447ed408c58c36cc8a251 9ec5ab5f85c8423145ea9d69eb6d848dfaa5c48c