diff --git a/bot/reviewbot/deprecation.py b/bot/reviewbot/deprecation.py index 8116c647163b68a3fbfe7e64d29be5f960e75ccc..76612a109bd370090dc2273de82462b29a0c5816 100644 --- a/bot/reviewbot/deprecation.py +++ b/bot/reviewbot/deprecation.py @@ -28,49 +28,23 @@ @final -class RemovedInReviewBot40Warning(BaseRemovedInReviewBotVersionWarning): - """Deprecations for features removed in Review Bot 4.0. - - Note that this class will itself be removed in Review Bot 4.0. If you need - to check against Review Bot deprecation warnings, please see - :py:class:`BaseRemovedInReviewBotVersionWarning`. - """ - - version = '4.0' - - -@final -class RemovedInReviewBot50Warning(BaseRemovedInReviewBotVersionWarning): - """Deprecations for features removed in Review Bot 5.0. - - Note that this class will itself be removed in Review Bot 5.0. If you need +class RemovedInReviewBot60Warning(BaseRemovedInReviewBotVersionWarning): + """Deprecations for features removed in Review Bot 6.0. + + Note that this class will itself be removed in Review Bot 6.0. If you need to check against Review Bot deprecation warnings, please see :py:class:`BaseRemovedInReviewBotVersionWarning`. Alternatively, you can use the alias for this class, :py:data:`RemovedInNextReviewBotVersionWarning`. """ - version = '5.0' - - -@final -class RemovedInReviewBot60Warning(BaseRemovedInReviewBotVersionWarning): - """Deprecations for features removed in Review Bot 6.0. - - Note that this class will itself be removed in Review Bot 6.0. If you need - to check against Review Bot deprecation warnings, please see - :py:class:`BaseRemovedInReviewBotVersionWarning`. - """ - version = '6.0' #: An alias for the next release of Review Bot where features would be removed. -RemovedInNextReviewBotVersionWarning = RemovedInReviewBot40Warning +RemovedInNextReviewBotVersionWarning = RemovedInReviewBot60Warning # Enable each warning for display. -for _warning_cls in (RemovedInReviewBot40Warning, - RemovedInReviewBot50Warning, - RemovedInReviewBot60Warning): +for _warning_cls in [RemovedInReviewBot60Warning]: warnings.simplefilter('once', _warning_cls, 0) diff --git a/bot/reviewbot/tests/test_tasks.py b/bot/reviewbot/tests/test_tasks.py index 3285f2ac10e8e8d4e2e1316b11d3afead9ac4fc2..d0b94b392e1db0f7a224e601a49767f23cd2ab34 100644 --- a/bot/reviewbot/tests/test_tasks.py +++ b/bot/reviewbot/tests/test_tasks.py @@ -12,7 +12,6 @@ from reviewbot.testing import TestCase from reviewbot.testing.testcases import (ReviewBotToolsResource, StatusUpdateResource) -from reviewbot.tools import Tool from reviewbot.tools.base import BaseTool from reviewbot.tools.base.registry import (_registered_tools, register_tool_class, @@ -20,13 +19,6 @@ from reviewbot.utils.api import get_api_root -class LegacyTool(Tool): - name = 'Legacy' - tool_id = 'legacy' - description = 'This is the legacy tool.' - timeout = 30 - - class DummyTool(BaseTool): name = 'Dummy' tool_id = 'dummy' @@ -63,7 +55,6 @@ _registered_tools.clear() register_tool_class(DummyTool) - register_tool_class(LegacyTool) register_tool_class(FullRepoTool) register_tool_class(FailedDepCheckTool) @@ -72,7 +63,6 @@ super(BaseTaskTestCase, cls).tearDownClass() unregister_tool_class(DummyTool.tool_id) - unregister_tool_class(LegacyTool.tool_id) unregister_tool_class(FullRepoTool.tool_id) unregister_tool_class(FailedDepCheckTool.tool_id) @@ -256,35 +246,6 @@ description='passed.') self.assertSpyCallCount(StatusUpdateResource.update, 2) - def test_with_legacy_tool(self): - """Testing RunTool task with legacy tool""" - self.spy_on(LegacyTool.execute, - owner=LegacyTool) - - result = self.run_tools_task( - routing_key=LegacyTool.tool_id, - tool_options={ - 'option1': 'value1', - 'option2': 'value2', - }) - - self.assertTrue(result) - self.assertSpyCalledWith( - LegacyTool.execute, - base_commit_id='', - repository=None, - settings={ - 'option1': 'value1', - 'option2': 'value2', - }) - - self.assertSpyCalledWith(StatusUpdateResource.update, - description='running...') - self.assertSpyCalledWith(StatusUpdateResource.update, - state='done-success', - description='passed.') - self.assertSpyCallCount(StatusUpdateResource.update, 2) - def test_with_base_commit_id(self): """Testing RunTool task with base_commit_id""" result = self.run_tools_task(base_commit_id='abc123', @@ -501,15 +462,6 @@ 'version': '2', 'working_directory_required': True, }, - { - 'description': 'This is the legacy tool.', - 'entry_point': 'legacy', - 'name': 'Legacy', - 'timeout': 30, - 'tool_options': '[]', - 'version': '1', - 'working_directory_required': False, - }, ], }) diff --git a/bot/reviewbot/tools/__init__.py b/bot/reviewbot/tools/__init__.py index 1e44e2b66d3e98ef4f582122fbdab9b416cf5e9d..f56fafa007d85069a36d40a8091a452e2ef06552 100644 --- a/bot/reviewbot/tools/__init__.py +++ b/bot/reviewbot/tools/__init__.py @@ -1,112 +1,10 @@ """Base support for code checking tools.""" -from __future__ import annotations - -from reviewbot.deprecation import RemovedInReviewBot40Warning -from reviewbot.tools.base import BaseTool, FullRepositoryToolMixin - - -class Tool(BaseTool): - """Legacy base class for tools. - - Deprecated: - 3.0: - Subclasses should instead inherit from - :py:class:`reviewbot.tools.base.tool.BaseTool` (or a more specific - subclass). - - This will be removed in Review Bot 4.0. - """ - - #: Internal state for marking this as a legacy tool. - #: - #: Do not change this. It is necessary for legacy tools to continue to - #: work in Review Bot 3.0. - #: - #: Version Added: - #: 3.0 - #: - #: Type: - #: bool - legacy_tool = True - - def __new__(cls, *args, **kwargs): - """Create an instance of the tool. - - This will emit a deprecation warning, warning of impending removal - and the changes that will be needed. - - Args: - *args (tuple): - Positional arguments to pass to the constructor. - - **kwargs (dict): - Keyword arguments to pass to the constructor. - - Returns: - Tool: - A new instance of the tool. - """ - RemovedInReviewBot40Warning.warn( - '%s must subclass reviewbot.tools.base.BaseTool. All ' - 'overridden methods, including __init__() and handle_file(), ' - 'must take a **kwargs argument, and self.settings should be ' - 'accessed for tool-specific settings. Legacy support will be ' - 'removed in Review Bot 4.0.' - % cls.__name__) - - return super(Tool, cls).__new__(cls) - - -class RepositoryTool(FullRepositoryToolMixin, BaseTool): - """Legacy base class for tools that need access to the entire repository. - - Deprecated: - 3.0: - Subclasses should instead inherit from - :py:class:`reviewbot.tools.base.tool.BaseTool` (or a more specific - subclass) and mix in - :py:class:`reviewbot.tools.base.mixins.FullRepositoryToolMixin`. - - This will be removed in Review Bot 4.0. - """ - - #: Internal state for marking this as a legacy tool. - #: - #: Do not change this. It is necessary for legacy tools to continue to - #: work in Review Bot 3.0. - #: - #: Version Added: - #: 3.0 - #: - #: Type: - #: bool - legacy_tool = True - - def __new__(cls, *args, **kwargs): - """Create an instance of the tool. - - This will emit a deprecation warning, warning of impending removal - and the changes that will be needed. - - Args: - *args (tuple): - Positional arguments to pass to the constructor. - - **kwargs (dict): - Keyword arguments to pass to the constructor. - - Returns: - Tool: - A new instance of the tool. - """ - RemovedInReviewBot40Warning.warn( - '%s must subclass reviewbot.tools.base.BaseTool, and mix in ' - 'reviewbot.tools.base.mixins.FullRepositoryToolMixin. All ' - 'overridden methods, including __init__() and handle_file(), ' - 'must take a **kwargs argument, and self.settings should be ' - 'accessed for tool-specific settings. Legacy support will be ' - 'removed in Review Bot 4.0.' - % cls.__name__) - - return super(RepositoryTool, cls).__new__(cls) +from reviewbot.tools.base import BaseTool + + +__all__ = [ + 'BaseTool', +] + +__autodoc_excludes__ = __all__ diff --git a/bot/reviewbot/tools/base/tool.py b/bot/reviewbot/tools/base/tool.py index fd1eae941305884c85682dd2fffc3f00071fa3d1..a6c22b2ca388caf0c6f33a9c9bce762ebab638b6 100644 --- a/bot/reviewbot/tools/base/tool.py +++ b/bot/reviewbot/tools/base/tool.py @@ -265,11 +265,10 @@ **kwargs (dict, unused): Additional keyword arguments, for future expansion. """ - if not getattr(self, 'legacy_tool', False): - kwargs.update({ - 'base_command': self.build_base_command(), - 'review': review, - }) + kwargs.update({ + 'base_command': self.build_base_command(), + 'review': review, + }) self.handle_files(review.files, **kwargs) @@ -316,17 +315,12 @@ Additional keyword arguments passed to :py:meth:`execute`. This is intended for future expansion. """ - legacy_tool = getattr(self, 'legacy_tool', False) - for f in files: if self.get_can_handle_file(review_file=f, **kwargs): - if legacy_tool: - self.handle_file(f, **kwargs) - else: - path = f.get_patched_file_path() + path = f.get_patched_file_path() - if path: - self.handle_file(f, path=path, **kwargs) + if path: + self.handle_file(f, path=path, **kwargs) def handle_file(self, f, path=None, base_command=None, **kwargs): """Perform a review of a single file. diff --git a/bot/reviewbot/tools/tests/test_base_tool.py b/bot/reviewbot/tools/tests/test_base_tool.py index 398b219a4a6cd1000d8ac75302b0c7b8dd956770..c559f4158ce3547149c04e4bf1ea2c068ae8022f 100644 --- a/bot/reviewbot/tools/tests/test_base_tool.py +++ b/bot/reviewbot/tools/tests/test_base_tool.py @@ -10,7 +10,7 @@ import kgb from reviewbot.testing import TestCase -from reviewbot.tools import BaseTool, Tool +from reviewbot.tools.base import BaseTool class DummyTool(BaseTool): @@ -159,34 +159,6 @@ self.assertSpyCalledWith(tool.handle_file, review_file2) self.assertSpyCallCount(tool.handle_file, 1) - def test_handle_files_with_patched_file_none_and_legacy_tool(self): - """Testing BaseTool.handle_files with file.get_patched_file_path() as - None and legacy tool - """ - class LegacyTool(Tool): - pass - - tool = LegacyTool() - - review = self.create_review() - review_file1 = self.create_review_file(review, - dest_file='/src/foo.c') - review_file2 = self.create_review_file(review, - dest_file='/inc/bar.cc') - - self.spy_on(tool.handle_file) - self.spy_on(review_file1.get_patched_file_path, - op=kgb.SpyOpReturn(None)) - self.spy_on(review_file2.get_patched_file_path) - - tool.handle_files(review.files) - - self.assertSpyNotCalled(review_file1.get_patched_file_path) - self.assertSpyNotCalled(review_file2.get_patched_file_path) - self.assertSpyCalledWith(tool.handle_file, review_file1) - self.assertSpyCalledWith(tool.handle_file, review_file2) - self.assertSpyCallCount(tool.handle_file, 2) - @contextmanager def _setup_deps(cls, filenames=[], set_path=True): """Set up an environment for dependency checks.