Refactor SCM tool registration using a registry
Review Request #9724 — Created March 3, 2018 and discarded
Instead of registering SCM tools in the database, populate a registry
at startup. This allows for more flexibility for users to register and
unregister external tools.Things done:
- Rename keys in registry to match the names in SCM tools classes
- Created service.py file with API for registering/unregistering tools
- Created test_registration.py to test service.py
- RenamedTool
model toLegacyTool
- Created new classTool
which is not a model. Instantiated by passing
in an SCM tool name (string). This class has the same properties as
LegacyTool
but gets the SCM tool class from the registry.
- Renamedtool
field inRepository
model to_legacy_tool
and add a
scmtool_name
field. Created an evolution for this.
- Createdtool
property inRepository
which does a lazy migration
away fromLegacyTool
by checking the value ofscmtool_name
.
- Replace usages of the legacytool
to usescmtool_name
E.g.tool = LegacyTool.objects.get(name='Subversion')
to
scmtool_name='Subversion'
- RefactorRepositoryForm
to usescmtool_name
- Other misc refactoring to pass tests inscmtools
,hostingsvcs
,
webapi
,reviews
, anddiffviewer
- Addmigratescmtools.py
command.
- Add SCMToolHook extension and unit tests
Currently passing all tests in
scmtools
,hostingsvcs
,webapi
,
reviews
, anddiffviewer
.Tested
migratescmtools.py
:
- Add local repos at/admin/db/scmtools/repository/add/
- Since theRepositoryForm
usesscmtool_name
now, manually
update repos in Django shell to be "pre-migration"
- Check that reposcmtool_name
and_legacy_tool
fields are as
expected post-migrationUnit tests for SCMToolHook extension.
Description | From | Last Updated |
---|---|---|
I'm wondering if this affects RBTools? I tried rbt patch to run the RBTools unit tests but applying the patch … |
MA maram | |
There should be one empty line after the doc string. |
RI ridum | |
There should be one empty line after the doc string. |
RI ridum | |
We need a whitespace between the doc String and the code |
RI ridum | |
There should be one empty line after the doc string. |
RI ridum | |
only one empty line is needed after yield |
RI ridum | |
F401 'pkg_resources' imported but unused |
reviewbot | |
F401 'sys' imported but unused |
reviewbot | |
Since this is a big project the class "command" may be a little ambiguious |
RI ridum | |
There should be one empty line after the doc string. |
RI ridum | |
NOTE: For some reason, the diff captured changes in whitespace which is why there are so many line chaneges in … |
JT jtrang | |
Any specific reason to use " over '? |
RO rodgerwa | |
Is there reason to keep this import here and commented out instead of just removing it? |
JC jcorrive | |
Is this commented line of code here for a purpose? |
JC jcorrive | |
F401 'reviewboard.scmtools.service.unregister_scm_tool' imported but unused |
reviewbot | |
F401 'reviewboard.scmtools.service.register_scm_tool' imported but unused |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
Has this been completed? |
JQ jque | |
I think this should be tool? |
JQ jque | |
Might need Args for name |
JQ jque | |
Can remove blank lines |
JQ jque | |
Spacing seemed fine before change |
JQ jque | |
These two lines should be switched so the imports are in alphabetical order. |
JC jcorrive | |
setUp -> setup |
FL floriecai | |
tearDown -> teardown |
FL floriecai | |
trailing white space |
FL floriecai | |
What is reverse relation here? |
FL floriecai | |
cls is a "reserved" (emphasis on the quotes there) word. Consider changing the name? See @classmethods |
rpolyano | |
See previous comment about cls (especially here used as an argument) |
rpolyano | |
tearDown -> teardown |
FL floriecai | |
setup_class() |
FL floriecai | |
Could you explain what the q & Q does? I saw this elsewhere and was confused by the syntax. |
FL floriecai |
Description: |
|
---|
Testing Done: |
|
---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+3067 -2804) |
Checks run (2 succeeded)
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+3090 -2804) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 3) -
reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 3) F401 'sys' imported but unused
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+3086 -2804) |
Checks run (2 succeeded)
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+3099 -2804) |
Checks run (2 succeeded)
-
-
docs/manual/fixtures/initial_data.json (Diff revision 5) NOTE: For some reason, the diff captured changes in whitespace which is why there are so many line chaneges in the diff. I only changed the following lines:
"model": "scmtools.legacytool"
and
"scmtool_name": "Test",
-
Mostly just questions.
-
-
reviewboard/scmtools/models.py (Diff revision 5) Why don't you use a lambda to assign this like the others?
IE: Move this with the other instance variables and have the "get_scmtool_class" return the assignment?
-
-
The code is generally detailed and easy to understand; but it will be nice to make sure that there should be a blank line after the last """ and the rest of the module/class body.
-
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4) There should be one empty line after the doc string.
-
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4) There should be one empty line after the doc string.
-
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4) We need a whitespace between the doc String and the code
-
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4) There should be one empty line after the doc string.
-
reviewboard/webapi/resources/repository.py (Diff revisions 2 - 4) only one empty line is needed after yield
-
reviewboard/scmtools/management/commands/migratescmtools.py (Diff revisions 4 - 5) Since this is a big project the class "command" may be a little ambiguious
-
reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 4) There should be one empty line after the doc string.
-
-
reviewboard/scmtools/service.py (Diff revision 5) Is there reason to keep this import here and commented out instead of just removing it?
-
reviewboard/scmtools/tests/test_bazaar.py (Diff revision 5) Is this commented line of code here for a purpose?
Change Summary:
Add whitespace after docstrings and remove some commented lines.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+3102 -2805) |
Checks run (2 succeeded)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+3102 -2805) |
Checks run (2 succeeded)
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+3168 -2806) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/extensions/tests.py (Diff revision 8) F401 'reviewboard.scmtools.service.unregister_scm_tool' imported but unused
-
reviewboard/extensions/tests.py (Diff revision 8) F401 'reviewboard.scmtools.service.register_scm_tool' imported but unused
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+3155 -2807) |
Checks run (2 succeeded)
-
Overall great job! Looks good from what I can tell. Mostly just found docstring and spacing issues.
-
-
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+3158 -2807) |
Checks run (2 timed out)
Testing Done: |
|
---|
Testing Done: |
|
---|
-
-
reviewboard/extensions/tests.py (Diff revision 10) These two lines should be switched so the imports are in alphabetical order.
-
-
reviewboard/scmtools/service.py (Diff revision 10) cls is a "reserved" (emphasis on the quotes there) word. Consider changing the name? See @classmethods
-
reviewboard/scmtools/service.py (Diff revision 10) See previous comment about cls (especially here used as an argument)
-
-
-
-
-
reviewboard/scmtools/management/commands/migratescmtools.py (Diff revision 10) What is reverse relation here?
-
-
-
reviewboard/webapi/resources/repository.py (Diff revision 10) Could you explain what the
q & Q
does? I saw this elsewhere and was confused by the syntax.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+3158 -2807) |