flake8
-
reviewboard/scmtools/registry.py (Diff revision 1) Show all issues
Review Request #12333 — Created June 3, 2022 and submitted
Information | |
---|---|
david | |
Review Board | |
release-5.0.x | |
Reviewers | |
reviewboard | |
The way that SCMTools are kept track of is particularly old. From the
very earliest days of Review Board, we knew that we wanted to support
multiple different SCMs, and we had done this by creating aTool
model
which had a name and the module path of a class to instantiate. This was
populated via database fixtures, andRepository
then had a relation to
the tool.As we improved things and made everything extensible, we moved away from
fixtures and over to entry points, but this still had two major
problems:
- If there's a
VersionConflict
in the install, no entry points will
load.- There was still a manual step that had to happen to load the tool
types from the entry points and populate theTool
table.
Occasionally, usually due to problem #1, people would end up with an
install that didn't have any Tool models, and we'd have to direct
them to manually run theregisterscmtools
command.This change is the first major step towards modernizing this entire part
of the system. This adds a new registry for storing the SCMTool classes.
By default, this is populated from three places. First, we load the
built-in SCMTools (whether or not entry points can load). Second, we
load anything present in entry points. Finally, we'll go through the
existing Tool table, and load anything else from there (while printing a
warning that said table is going away, and anything there will need to
be changed to use entry points or theregister()
method directly.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
E501 line too long (82 > 79 characters) |
![]() |
|
Can you move these above Args? I think I did this, but they weren't supposed to be here. Also the … |
|
|
So it's mixed, but for the most part, we suffix our registries with _registry. I think that might be good … |
|
|
Do we need to do this? populate() should be called on first access to a registry. |
|
|
Long-term, I'd love to nuke the entrypoint support here. They're more problematic than anything, and it's better to control instantiation/registration … |
|
|
We could save the final lookup by just loading everything for Tool up-front into an explicit list, adding anything we … |
|
|
We should use RemovedInReviewBoard060Warning.warn(...) rather than warnings. You can pass a stacklevel=1 if you want to use that. |
|
|
We should really require (or at least guide) registration and not entrypoints. |
|
|
This should be an explicit logging statement. Warnings aren't guaranteed to be shown or end up in the server log. |
|
|
This can be Tool.objects.create(). No need for .save(). |
|
|
What does this raise? |
|
|
Same question here. |
|
|
'warnings' imported but unused Column: 1 Error code: F401 |
![]() |
|
Small nit: Let's make this a set, so lookups in the block below are just a tad more efficient. |
|
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+522 -48) |
reviewboard/__init__.py (Diff revision 2) |
---|
Can you move these above
Args
? I think I did this, but they weren't supposed to be here.Also the
4.0
shouldn't have a:
after it, as the parser will expect a second line with a description.No indentation on that description line for
5.0
.Also should be in order of newest to oldest.
reviewboard/scmtools/__init__.py (Diff revision 2) |
---|
So it's mixed, but for the most part, we suffix our registries with
_registry
. I think that might be good to do here, especially given that this is the same name as the module it's in.
reviewboard/scmtools/__init__.py (Diff revision 2) |
---|
Do we need to do this?
populate()
should be called on first access to a registry.
reviewboard/scmtools/registry.py (Diff revision 2) |
---|
Long-term, I'd love to nuke the entrypoint support here. They're more problematic than anything, and it's better to control instantiation/registration through extensions.
Short-term, I know we need it.
If you're on board with the long-term goal, can we document in the docstring that entrypoint support is deprecated? We'd also need to emit a deprecation warning on anything we load.
reviewboard/scmtools/registry.py (Diff revision 2) |
---|
We could save the final lookup by just loading everything for
Tool
up-front into an explicitlist
, adding anything we create to that list, and then iterating over the result.
reviewboard/scmtools/registry.py (Diff revision 2) |
---|
We should use
RemovedInReviewBoard060Warning.warn(...)
rather thanwarnings
.You can pass a
stacklevel=1
if you want to use that.
reviewboard/scmtools/registry.py (Diff revision 2) |
---|
We should really require (or at least guide) registration and not entrypoints.
reviewboard/scmtools/registry.py (Diff revision 2) |
---|
This should be an explicit logging statement. Warnings aren't guaranteed to be shown or end up in the server log.
reviewboard/scmtools/registry.py (Diff revision 2) |
---|
This can be
Tool.objects.create()
. No need for.save()
.
AppConfig.ready
method instead of using initializing
.Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+584 -54) |
reviewboard/scmtools/registry.py (Diff revision 3) |
---|
'warnings' imported but unused Column: 1 Error code: F401
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+582 -54) |
reviewboard/scmtools/registry.py (Diff revision 4) |
---|
Small nit: Let's make this a
set
, so lookups in the block below are just a tad more efficient.