diff --git a/reviewboard/admin/checks.py b/reviewboard/admin/checks.py index c6adca9857b53c11f829c120c2adbe0e511a58d0..70669cc2320cf87bb0740ed65391addc3000db06 100644 --- a/reviewboard/admin/checks.py +++ b/reviewboard/admin/checks.py @@ -41,7 +41,6 @@ from djblets.siteconfig.models import SiteConfiguration import reviewboard from reviewboard import get_version_string from reviewboard.admin.import_utils import has_module -from reviewboard.deprecation import RemovedInReviewBoard60Warning _install_fine = False @@ -246,24 +245,6 @@ def get_can_enable_ldap(): )) -def get_can_enable_dns(): - """Return whether DNS querying support can be enabled. - - Deprecated: - 4.0: - This is now always true, as ``dnspython`` is a required dependency. - - Returns: - tuple: - ``(True, None)``, always. - """ - RemovedInReviewBoard60Warning.warn( - 'As of Review Board 4.0, get_can_enable_dns() always returns a true ' - 'result. It will be removed in Review Board 6.0.') - - return True, None - - def get_can_use_amazon_s3(): """Check whether django-storages (Amazon S3 backend) is installed.""" try: diff --git a/reviewboard/admin/forms/search_settings.py b/reviewboard/admin/forms/search_settings.py index 2ab843edb525bf7cf288ffb45a97962e3955da4e..4adb5a690f43b0ac0014ee5039193ce072d31a28 100644 --- a/reviewboard/admin/forms/search_settings.py +++ b/reviewboard/admin/forms/search_settings.py @@ -7,7 +7,6 @@ from django.utils.translation import gettext, gettext_lazy as _ from djblets.siteconfig.forms import SiteSettingsForm from reviewboard.admin.siteconfig import load_site_config -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.search import search_backend_registry @@ -173,16 +172,7 @@ class SearchSettingsForm(SiteSettingsForm): backend_form.cleaned_data) try: - if func_accepts_kwargs(search_backend.validate): - search_backend.validate( - configuration=configuration) - else: - RemovedInReviewBoard60Warning.warn( - '%s.validate() must accept keyword ' - 'arguments. This will be required in ' - 'Review Board 6.0.' - % search_backend.__class__.__name__) - search_backend.validate() + search_backend.validate(configuration=configuration) except ValidationError as e: self.add_error('search_backend_id', e.error_list) else: diff --git a/reviewboard/admin/tests/test_search_settings_form.py b/reviewboard/admin/tests/test_search_settings_form.py index 632f7ce38298dd8b9a5ea89723634c288b6595ec..3c64ebcb5eedd53b8900f31be58728d5bb9edcfb 100644 --- a/reviewboard/admin/tests/test_search_settings_form.py +++ b/reviewboard/admin/tests/test_search_settings_form.py @@ -7,7 +7,6 @@ from django.forms import ValidationError from djblets.siteconfig.models import SiteConfiguration from reviewboard.admin.forms.search_settings import SearchSettingsForm -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.search import search_backend_registry from reviewboard.search.search_backends.base import (SearchBackend, SearchBackendForm) @@ -84,44 +83,6 @@ class SearchSettingsFormTests(TestCase): finally: search_backend_registry.unregister(backend) - def test_clean_with_backend_validate_legacy_fail(self): - """Testing SearchSettingsForm.clean when the backend doesn't pass - validation - """ - class InvalidSearchBackendForm(SearchBackendForm): - pass - - class InvalidSearchBackend(SearchBackend): - search_backend_id = 'invalid' - config_form_class = InvalidSearchBackendForm - - def validate(self): - raise ValidationError('This backend is invalid.') - - backend = InvalidSearchBackend() - search_backend_registry.register(backend) - - try: - siteconfig = SiteConfiguration.objects.get_current() - form = SearchSettingsForm(siteconfig, data={ - 'search_enable': True, - 'search_backend_id': backend.search_backend_id, - }) - - message = ( - 'InvalidSearchBackend.validate() must accept keyword ' - 'arguments. This will be required in Review Board 6.0.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - self.assertFalse(form.is_valid()) - - self.assertIn('search_backend_id', form.errors) - self.assertEqual(form.errors['search_backend_id'], - ['This backend is invalid.']) - finally: - search_backend_registry.unregister(backend) - def test_clean_missing_backend(self): """Testing SearchSettingsForm.clean when the backend doesn't exist""" siteconfig = SiteConfiguration.objects.get_current() diff --git a/reviewboard/admin/widgets.py b/reviewboard/admin/widgets.py index 05f195a06faabcc9c225e33bb47ce5c840ca1c4d..9c4b2c3554f642d336f8e48a13153ca42d846553 100644 --- a/reviewboard/admin/widgets.py +++ b/reviewboard/admin/widgets.py @@ -21,9 +21,7 @@ from djblets.util.decorators import augment_method_from from reviewboard import get_manual_url from reviewboard.admin.cache_stats import get_cache_stats from reviewboard.changedescs.models import ChangeDescription -from reviewboard.deprecation import RemovedInReviewBoard60Warning -from reviewboard.reviews.models import (ReviewRequest, Group, - Comment, Review) +from reviewboard.reviews.models import Comment, Group, Review, ReviewRequest from reviewboard.scmtools.models import Repository @@ -609,46 +607,5 @@ def init_widgets(): post_delete.connect(_increment_sync_num, sender=Repository) -def register_admin_widget(widget_cls, primary=False): - """Register an administration widget. - - Deprecated:: - 4.0: - Widgets should be registered on :py:data:`admin_widgets_registry` - instead. - - Args: - widget_cls (type): - The subclass of :py:class:`BaseAdminWidget` to register. - - primary (bool, optional): - Ignored. - """ - warnings.warn( - 'reviewboard.admin.widgets.register_admin_widget() is deprecated ' - 'and will be removed in Review Board 6.0. Use ' - 'reviewboard.admin.widgets.admin_widgets_registry.register() ' - 'to register %r instead.' - % widget_cls, - RemovedInReviewBoard60Warning, - stacklevel=2) - - admin_widgets_registry.register(widget_cls) - - -def unregister_admin_widget(widget_cls): - """Unregister a previously registered administration widget.""" - warnings.warn( - 'reviewboard.admin.widgets.unregister_admin_widget() is deprecated ' - 'and will be removed in Review Board 6.0. Use ' - 'reviewboard.admin.widgets.admin_widgets_registry.unregister() ' - 'to unregister %r instead.' - % widget_cls, - RemovedInReviewBoard60Warning, - stacklevel=2) - - admin_widgets_registry.unregister(widget_cls) - - #: The registry of available administration widgets. admin_widgets_registry = AdminWidgetsRegistry() diff --git a/reviewboard/deprecation.py b/reviewboard/deprecation.py index a4b0a9d38774fc77e97b09a9f4227e83b5e388d1..395407ca1257f6e4c1f76ab8158e11702b67dfb4 100644 --- a/reviewboard/deprecation.py +++ b/reviewboard/deprecation.py @@ -7,8 +7,11 @@ stable. import warnings +from djblets.deprecation import BaseRemovedInProductVersionWarning -class BaseRemovedInReviewBoardVersionWarning(DeprecationWarning): + +class BaseRemovedInReviewBoardVersionWarning( + BaseRemovedInProductVersionWarning): """Base class for a Review Board deprecation warning. All version-specific deprecation warnings inherit from this, allowing @@ -16,33 +19,7 @@ class BaseRemovedInReviewBoardVersionWarning(DeprecationWarning): specific version. """ - @classmethod - def warn(cls, message, stacklevel=2): - """Emit the deprecation warning. - - This is a convenience function that emits a deprecation warning using - this class, with a suitable default stack level. Callers can provide - a useful message and a custom stack level. - - Args: - message (unicode): - The message to show in the deprecation warning. - - stacklevel (int, optional): - The stack level for the warning. - """ - warnings.warn(message, cls, stacklevel=stacklevel + 1) - - -class RemovedInReviewBoard60Warning(BaseRemovedInReviewBoardVersionWarning): - """Deprecations for features scheduled for removal in Review Board 6.0. - - Note that this class will itself be removed in Review Board 6.0. If you - need to check against Review Board deprecation warnings, please see - :py:class:`BaseRemovedInReviewBoardVersionWarning`. Alternatively, you - can use the alias for this class, - :py:data:`RemovedInNextReviewBoardVersionWarning`. - """ + product = 'Review Board' class RemovedInReviewBoard70Warning(BaseRemovedInReviewBoardVersionWarning): @@ -55,7 +32,9 @@ class RemovedInReviewBoard70Warning(BaseRemovedInReviewBoardVersionWarning): :py:data:`RemovedInNextReviewBoardVersionWarning`. """ + version = '7.0' + #: An alias for the next release of Review Board where features will be #: removed. -RemovedInNextReviewBoardVersionWarning = RemovedInReviewBoard60Warning +RemovedInNextReviewBoardVersionWarning = RemovedInReviewBoard70Warning diff --git a/reviewboard/diffviewer/chunk_generator.py b/reviewboard/diffviewer/chunk_generator.py index db15f13da068e3088e6ae99b73ee362fcb373097..e9572421464395a98377889f3a5954367ad84b15 100644 --- a/reviewboard/diffviewer/chunk_generator.py +++ b/reviewboard/diffviewer/chunk_generator.py @@ -11,6 +11,7 @@ from django.utils.html import escape from django.utils.translation import get_language, gettext as _ from djblets.log import log_timed from djblets.cache.backend import cache_memoize +from djblets.deprecation import deprecate_non_keyword_only_args from djblets.siteconfig.models import SiteConfiguration from pygments import highlight from pygments.formatters import HtmlFormatter @@ -18,7 +19,7 @@ from pygments.lexers import (find_lexer_class, guess_lexer_for_filename) from reviewboard.codesafety import code_safety_checker_registry -from reviewboard.deprecation import RemovedInReviewBoard60Warning +from reviewboard.deprecation import RemovedInReviewBoard70Warning from reviewboard.diffviewer.differ import DiffCompatVersion, get_differ from reviewboard.diffviewer.diffutils import (get_filediff_encodings, get_line_changed_regions, @@ -119,12 +120,22 @@ class RawDiffChunkGenerator(object): #: reviewboard.diffviewer.settings.DiffSettings diff_settings: DiffSettings - def __init__(self, old, new, orig_filename, modified_filename, - enable_syntax_highlighting=None, encoding_list=None, + def __init__(self, + old, + new, + orig_filename, + modified_filename, + encoding_list=None, diff_compat=DiffCompatVersion.DEFAULT, - *, diff_settings=None): + *, + diff_settings: DiffSettings): """Initialize the chunk generator. + Version Changed: + 6.0: + * Removed ``enable_syntax_highlighting``. + * Made ``diff_settings`` mandatory. + Version Changed: 5.0.2: * Added ``diff_settings``, which will be required starting in @@ -145,14 +156,6 @@ class RawDiffChunkGenerator(object): modified_filename (unicode): The filename corresponding to the new data. - enable_syntax_highlighting (bool, optional): - Whether to default to enabling syntax highlighting if - ``diff_settings`` is not provided. - - Deprecated: - 5.0.2: - This has been replaced with ``diff_settings``. - encoding_list (list of unicode, optional): A list of encodings to try for the ``old`` and ``new`` data, when converting to Unicode. If not specified, this defaults @@ -194,22 +197,6 @@ class RawDiffChunkGenerator(object): _('%s expects a Unicode value for "modified_filename"') % type(self).__name__) - if enable_syntax_highlighting is not None: - RemovedInReviewBoard60Warning.warn( - 'The `enable_syntax_highlighting` argument to %r is ' - 'deprecated and will be removed in Review Board 6.0. ' - 'Please provide `diff_settings` instead.' - % type(self)) - - if diff_settings is None: - diff_settings = DiffSettings.create() - - # Satisfy the type checker, due to the parameter being optional. - assert diff_settings is not None - - if enable_syntax_highlighting is not None: - diff_settings.syntax_highlighting = enable_syntax_highlighting - self.old = old self.new = new self.orig_filename = orig_filename @@ -1132,12 +1119,22 @@ class DiffChunkGenerator(RawDiffChunkGenerator): grab a patched file for the interdiff version. """ - def __init__(self, request, filediff, interfilediff=None, - force_interdiff=False, enable_syntax_highlighting=None, + @deprecate_non_keyword_only_args(RemovedInReviewBoard70Warning) + def __init__(self, + request, + filediff, + interfilediff=None, + force_interdiff=False, base_filediff=None, - *, diff_settings=None): + *, + diff_settings): """Initialize the DiffChunkGenerator. + Version Changed: + 6.0: + * Removed the old ``enable_syntax_highlighting`` argument. + * Made ``diff_settings`` mandatory. + Args: request (django.http.HttpRequest): The HTTP request from the client. @@ -1154,15 +1151,14 @@ class DiffChunkGenerator(RawDiffChunkGenerator): force_interdiff (bool, optional): Whether or not to force an interdiff. - enable_syntax_highlighting (bool, optional): - Whether or not to enable syntax highlighting. - base_filediff (reviewboard.diffviewer.models.filediff.FileDiff, optional): An ancestor of ``filediff`` that we want to use as the base. Using this argument will result in the history between ``base_filediff`` and ``filediff`` being applied. + diff_settings (reviewboard.diffviewer.settings.DiffSettings): + The settings used to control the display of diffs. """ assert filediff diff --git a/reviewboard/diffviewer/diffutils.py b/reviewboard/diffviewer/diffutils.py index e2dc734544b269888422ad58440c7e88f592b77d..8e30e6d86e95d29a28f0f43cb944c1047faa8512 100644 --- a/reviewboard/diffviewer/diffutils.py +++ b/reviewboard/diffviewer/diffutils.py @@ -11,12 +11,13 @@ from functools import cmp_to_key from django.core.exceptions import ObjectDoesNotExist from django.utils.encoding import force_str from django.utils.translation import gettext as _ +from djblets.deprecation import deprecate_non_keyword_only_args from djblets.log import log_timed from djblets.siteconfig.models import SiteConfiguration from djblets.util.compat.python.past import cmp from djblets.util.contextmanagers import controlled_subprocess -from reviewboard.deprecation import RemovedInReviewBoard60Warning +from reviewboard.deprecation import RemovedInReviewBoard70Warning from reviewboard.diffviewer.commit_utils import exclude_ancestor_filediffs from reviewboard.diffviewer.errors import DiffTooBigError, PatchError from reviewboard.diffviewer.settings import DiffSettings @@ -1213,31 +1214,31 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, key=lambda f: f['interfilediff'] or f['filediff']) -def populate_diff_chunks(files, enable_syntax_highlighting=None, - request=None, - *, diff_settings=None): +@deprecate_non_keyword_only_args(RemovedInReviewBoard70Warning) +def populate_diff_chunks( + files, + *, + request=None, + diff_settings: DiffSettings): """Populate a list of diff files with chunk data. This accepts a list of files (generated by :py:func:`get_diff_files`) and generates diff chunk data for each file in the list. The chunk data is stored in memory in the file state. + Version Changed: + 6.0: + * Made all arguments other than ``files`` keyword-only. + * Made the ``diff_settings`` argument mandatory. + Args: files (list of dict): The list of diff files to generate chunks for. - enable_syntax_highlighting (bool, optional): - Whether to default to enabling syntax highlighting if - ``diff_settings`` is not provided. - - Deprecated: - 5.0.2: - This has been replaced with ``diff_settings``. - request (django.http.HttpRequest, optional): The HTTP request from the client. - diff_settings (reviewboard.diffviewer.settings.DiffSettings, optional): + diff_settings (reviewboard.diffviewer.settings.DiffSettings): The settings used to control the display of diffs. Version Added: @@ -1245,21 +1246,6 @@ def populate_diff_chunks(files, enable_syntax_highlighting=None, """ from reviewboard.diffviewer.chunk_generator import get_diff_chunk_generator - if enable_syntax_highlighting is not None: - RemovedInReviewBoard60Warning.warn( - 'The `enable_syntax_highlighting` argument to ' - 'populate_diff_chunks() is deprecated and will be removed in ' - 'Review Board 6.0. Provide `diff_settings` instead.') - - if diff_settings is None: - diff_settings = DiffSettings.create(request=request) - - # Satisfy the type checker, due to the parameter being optional. - assert diff_settings is not None - - if enable_syntax_highlighting is not None: - diff_settings.syntax_highlighting = enable_syntax_highlighting - for diff_file in files: chunk_generator = get_diff_chunk_generator( request=request, @@ -1297,8 +1283,11 @@ def populate_diff_chunks(files, enable_syntax_highlighting=None, }) -def get_file_from_filediff(context, filediff, interfilediff, - *, diff_settings=None): +def get_file_from_filediff(context, + filediff, + interfilediff, + *, + diff_settings): """Return the files that corresponds to the filediff/interfilediff. This is primarily intended for use with templates. It takes a template @@ -1308,6 +1297,10 @@ def get_file_from_filediff(context, filediff, interfilediff, This function returns either exactly one file or ``None``. + Version Changed: + 6.0: + * Made ``diff_settings`` mandatory. + Version Changed: 5.0.2: * Added the optional ``diff_settings`` argument. @@ -1323,7 +1316,7 @@ def get_file_from_filediff(context, filediff, interfilediff, optional): The optional filediff being used to render an interdiff. - diff_settings (reviewboard.diffviewer.settings.DiffSettings, optional): + diff_settings (reviewboard.diffviewer.settings.DiffSettings): The settings used to control the display of diffs. Version Added: @@ -1350,6 +1343,7 @@ def get_file_from_filediff(context, filediff, interfilediff, files = get_diff_files(filediff.diffset, filediff, interdiffset, interfilediff=interfilediff, request=request) + populate_diff_chunks(files=files, request=request, diff_settings=diff_settings) @@ -1362,8 +1356,11 @@ def get_file_from_filediff(context, filediff, interfilediff, return None -def get_last_line_number_in_diff(context, filediff, interfilediff, - *, diff_settings=None): +def get_last_line_number_in_diff(context, + filediff, + interfilediff, + *, + diff_settings): """Return the last virtual line number in the filediff/interfilediff. This returns the virtual line number to be used in expandable diff @@ -1384,7 +1381,7 @@ def get_last_line_number_in_diff(context, filediff, interfilediff, optional): The optional filediff being used to render an interdiff. - diff_settings (reviewboard.diffviewer.settings.DiffSettings, optional): + diff_settings (reviewboard.diffviewer.settings.DiffSettings): The settings used to control the display of diffs. Version Added: @@ -1496,8 +1493,12 @@ def _get_last_header_in_chunks_before_line(chunks, target_line): return header -def get_last_header_before_line(context, filediff, interfilediff, target_line, - *, diff_settings=None): +def get_last_header_before_line(context, + filediff, + interfilediff, + target_line, + *, + diff_settings): """Return the last header that occurs before the given line. Version Changed: @@ -1517,7 +1518,7 @@ def get_last_header_before_line(context, filediff, interfilediff, target_line, target_line (int): The virtual line number that the header must be before. - diff_settings (reviewboard.diffviewer.settings.DiffSettings, optional): + diff_settings (reviewboard.diffviewer.settings.DiffSettings): The settings used to control the display of diffs. Version Added: @@ -1538,9 +1539,13 @@ def get_last_header_before_line(context, filediff, interfilediff, target_line, target_line=target_line) -def get_file_chunks_in_range(context, filediff, interfilediff, - first_line, num_lines, - *, diff_settings=None): +def get_file_chunks_in_range(context, + filediff, + interfilediff, + first_line, + num_lines, + *, + diff_settings): """Generate the chunks within a range of lines in the specified filediff. This is primarily intended for use with templates. It takes a template @@ -1571,7 +1576,7 @@ def get_file_chunks_in_range(context, filediff, interfilediff, num_lines (int): The number of lines in the range. - diff_settings (reviewboard.diffviewer.settings.DiffSettings, optional): + diff_settings (reviewboard.diffviewer.settings.DiffSettings): The settings used to control the display of diffs. Version Added: @@ -1654,44 +1659,6 @@ def get_chunks_in_range(chunks, first_line, num_lines): break -def get_enable_highlighting(user): - """Return whether syntax highlighting should be enabled for a user. - - Syntax highlighting is only enabled if turned on server-wide, and if the - user hasn't disabled it. - - Deprecated: - 5.0.2: - This is deprecated and will be removed in Review Board 6.0. Use - :py:class:`reviewboard.diffviewer.settings.DiffSettings` instead. - - Args: - user (django.contrib.auth.models.AbstractUser, optional): - The user to check. - - Returns: - bool: - ``True`` if syntax highlighting is enabled. ``False`` if it is not. - """ - RemovedInReviewBoard60Warning.warn( - 'get_enable_highlighting() is deprecated. Please use ' - 'reviewboard.diffviewer.settings.DiffSettings instead. This will ' - 'be removed in Review Board 6.0.') - - user_syntax_highlighting = True - - if user.is_authenticated: - try: - profile = user.get_profile() - user_syntax_highlighting = profile.syntax_highlighting - except ObjectDoesNotExist: - pass - - siteconfig = SiteConfiguration.objects.get_current() - return (siteconfig.get('diffviewer_syntax_highlighting') and - user_syntax_highlighting) - - def get_line_changed_regions(oldline, newline): """Returns regions of changes between two similar lines.""" if oldline is None or newline is None: diff --git a/reviewboard/diffviewer/parser.py b/reviewboard/diffviewer/parser.py index 8608cac5816f571db509ae78f1b17f477b82eadf..19914153637196e9899ce50ee416f58864c9f634 100644 --- a/reviewboard/diffviewer/parser.py +++ b/reviewboard/diffviewer/parser.py @@ -10,7 +10,6 @@ from djblets.util.properties import AliasProperty, TypedProperty from pydiffx import DiffType, DiffX from pydiffx.errors import DiffXParseError -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.diffviewer.errors import DiffParserError from reviewboard.scmtools.core import HEAD, PRE_CREATION, Revision, UNKNOWN @@ -281,56 +280,6 @@ class ParsedDiffFile(object): #: int new_unix_mode = TypedProperty(str) - #: The parsed original name of the file. - #: - #: Deprecated: - #: 4.0: - #: Use :py:attr:`orig_filename` instead. - origFile = AliasProperty('orig_filename', - convert_to_func=force_bytes, - deprecated=True, - deprecation_warning=RemovedInReviewBoard60Warning) - - #: The parsed file details of the original file. - #: - #: Deprecated: - #: 4.0: - #: Use :py:attr:`orig_file_details` instead. - origInfo = AliasProperty('orig_file_details', - convert_to_func=force_bytes, - deprecated=True, - deprecation_warning=RemovedInReviewBoard60Warning) - - #: The parsed original name of the file. - #: - #: Deprecated: - #: 4.0: - #: Use :py:attr:`modified_filename` instead. - newFile = AliasProperty('modified_filename', - convert_to_func=force_bytes, - deprecated=True, - deprecation_warning=RemovedInReviewBoard60Warning) - - #: The parsed file details of the modified file. - #: - #: Deprecated: - #: 4.0: - #: Use :py:attr:`modified_file_details` instead. - newInfo = AliasProperty('modified_file_details', - convert_to_func=force_bytes, - deprecated=True, - deprecation_warning=RemovedInReviewBoard60Warning) - - #: The parsed value for an Index header. - #: - #: Deprecated: - #: 4.0: - #: Use :py:attr:`index_header_value` instead. - index = AliasProperty('index_header_value', - convert_to_func=force_bytes, - deprecated=True, - deprecation_warning=RemovedInReviewBoard60Warning) - def __init__(self, parser=None, parsed_diff_change=None, **kwargs): """Initialize the parsed file information. @@ -353,13 +302,6 @@ class ParsedDiffFile(object): This will be required in Review Board 6.0. """ - if parsed_diff_change is None: - RemovedInReviewBoard60Warning.warn( - 'Diff parsers must pass a ParsedDiffChange as the ' - 'parsed_diff_change= parameter when creating a ' - 'ParsedDiffFile. They should no longer pass a parser= ' - 'parameter. This will be mandatory in Review Board 6.0.') - if parsed_diff_change is not None: parsed_diff_change.files.append(self) parser = parsed_diff_change.parent_parsed_diff.parser @@ -381,8 +323,6 @@ class ParsedDiffFile(object): self._data_io = io.BytesIO() self._data = None - self._deprecated_info = {} - @property def parent_parsed_diff_change(self): """The parent change object. @@ -398,154 +338,6 @@ class ParsedDiffFile(object): return None - def __setitem__(self, key, value): - """Set information on the parsed file from a diff. - - This is a legacy implementation used to help diff parsers retain - compatibility with the old dictionary-based ways of setting parsed - file information. Callers should be updated to set attributes instead. - - Deprecated: - 4.0: - This will be removed in Review Board 6.0. - - Args: - key (str): - The key to set. - - value (object): - The value to set. - """ - self._warn_old_usage_deprecation() - - self._deprecated_info[key] = value - setattr(self, key, value) - - def __getitem__(self, key): - """Return information on the parsed file from a diff. - - This is a legacy implementation used to help diff parsers retain - compatibility with the old dictionary-based ways of setting parsed - file information. Callers should be updated to access attributes - instead. - - Deprecated: - 4.0: - This will be removed in Review Board 6.0. - - Args: - key (str): - The key to retrieve. - - Returns: - object: - The resulting value. - - Raises: - KeyError: - The key is invalid. - """ - self._warn_old_usage_deprecation() - - return self._deprecated_info[key] - - def __contains__(self, key): - """Return whether an old parsed file key has been explicitly set. - - This is a legacy implementation used to help diff parsers retain - compatibility with the old dictionary-based ways of setting parsed - file information. Callers should be updated to check attribute values - instead. - - Deprecated: - 4.0: - This will be removed in Review Board 6.0. - - Args: - key (str): - The key to check. - - Returns: - bool: - ``True`` if the key has been explicitly set by a diff parser. - ``False`` if it has not. - """ - self._warn_old_usage_deprecation() - - return key in self._deprecated_info - - def set(self, key, value): - """Set information on the parsed file from a diff. - - This is a legacy implementation used to help diff parsers retain - compatibility with the old dictionary-based ways of setting parsed - file information. Callers should be updated to set attributes instead. - - Deprecated: - 4.0: - This will be removed in Review Board 6.0. - - Args: - key (str): - The key to set. - - value (object): - The value to set. - """ - self._warn_old_usage_deprecation() - - self._deprecated_info[key] = value - setattr(self, key, value) - - def get(self, key, default=None): - """Return information on the parsed file from a diff. - - This is a legacy implementation used to help diff parsers retain - compatibility with the old dictionary-based ways of setting parsed - file information. Callers should be updated to access attributes - instead. - - Deprecated: - 4.0: - This will be removed in Review Board 6.0. - - Args: - key (str): - The key to retrieve. - - default (object, optional): - The default value to return. - - Returns: - object: - The resulting value. - """ - self._warn_old_usage_deprecation() - - return self._deprecated_info.get(key, default) - - def update(self, items): - """Update information on the parsed file from a diff. - - This is a legacy implementation used to help diff parsers retain - compatibility with the old dictionary-based ways of setting parsed - file information. Callers should be updated to set individual - attributes instead. - - Deprecated: - 4.0: - This will be removed in Review Board 6.0. - - Args: - items (dict): - The keys and values to set. - """ - self._warn_old_usage_deprecation() - - for key, value in items.items(): - self._deprecated_info[key] = value - setattr(self, key, value) - @property def data(self): """The data for this diff. @@ -606,24 +398,6 @@ class ParsedDiffFile(object): if data: self._data_io.write(data) - def _warn_old_usage_deprecation(self): - """Warn that a DiffParser is populating information in an old way.""" - if self.parser is None: - message = ( - 'Diff parsers must be updated to populate attributes on a ' - 'ParsedDiffFile, instead of setting the information in a ' - 'dictionary. This will be required in Review Board 6.0.' - ) - else: - message = ( - '%r must be updated to populate attributes on a ' - 'ParsedDiffFile, instead of setting the information in a ' - 'dictionary. This will be required in Review Board 6.0.' - % type(self.parser) - ) - - RemovedInReviewBoard60Warning.warn(message, stacklevel=3) - class BaseDiffParser(object): """Base class for a diff parser. @@ -775,7 +549,6 @@ class DiffParser(BaseDiffParser): * :py:meth:`parse_diff_header` * :py:meth:`parse_filename_header` * :py:meth:`parse_after_headers` - * :py:meth:`get_orig_commit_id` * :py:meth:`normalize_diff_filename` """ @@ -857,21 +630,6 @@ class DiffParser(BaseDiffParser): if parsed_diff_file.parent_parsed_diff_change is None: parsed_diff_change.files.append(parsed_diff_file) - if parsed_diff_change.parent_commit_id is None: - parent_commit_id = self.get_orig_commit_id() - - if parent_commit_id is not None: - parsed_diff_change.parent_commit_id = parent_commit_id - self.parsed_diff.uses_commit_ids_as_revisions = True - - RemovedInReviewBoard60Warning.warn( - '%s.get_orig_commit_id() will no longer be supported in ' - 'Review Board 6.0. Please set the commit ID in ' - 'self.parsed_diff_change.parent_commit_id, and set ' - 'parsed_diff_change.uses_commit_ids_as_revisions = True.' - % type(self).__name__ - ) - logger.debug('%s.parse_diff: Finished parsing diff.', class_name) return self.parsed_diff @@ -1106,12 +864,6 @@ class DiffParser(BaseDiffParser): try: parsed_file.index_header_value = \ index_line.split(None, 1)[1] - - # Set these for backwards-compatibility. - # - # This should be removed in Review Board 6.0. - parsed_file._deprecated_info['index'] = \ - parsed_file.index_header_value except ValueError: raise DiffParserError('Malformed Index line', linenum) @@ -1196,18 +948,6 @@ class DiffParser(BaseDiffParser): self.parse_filename_header(self.lines[linenum][4:], linenum) - # Set these for backwards-compatibility. - # - # This should be removed in Review Board 6.0. - parsed_file._deprecated_info['origFile'] = \ - parsed_file.orig_filename - parsed_file._deprecated_info['origInfo'] = \ - parsed_file.orig_file_details - parsed_file._deprecated_info['newFile'] = \ - parsed_file.modified_filename - parsed_file._deprecated_info['newInfo'] = \ - parsed_file.modified_file_details - linenum += 1 except ValueError: raise DiffParserError( @@ -1356,30 +1096,6 @@ class DiffParser(BaseDiffParser): for filediff in filediffs ) - def get_orig_commit_id(self): - """Return the commit ID of the original revision for the diff. - - By default, this returns ``None``. Subclasses would override this if - they work with repositories that always look up changes to a file by - the ID of the commit that made the changes instead of a per-file - revision or ID. - - Non-``None`` values returned by this method will override the values - being stored in :py:attr:`FileDiff.source_revision - `. - - Implementations would likely want to parse out the commit ID from - some prior header and return it here. By the time this is called, all - files will have been parsed already. - - Returns: - bytes: - The commit ID used to override the source revision of any created - :py:class:`~reviewboard.diffviewer.models.filediff.FileDiff` - instances. - """ - return None - class DiffXParser(BaseDiffParser): """Parser for DiffX files. diff --git a/reviewboard/diffviewer/renderers.py b/reviewboard/diffviewer/renderers.py index f9c201a5fbeaf41a2c2a4a0e84b6c84ed8ba5fdc..d1c7912fc52404145fe370fcfec025b7657e2d65 100644 --- a/reviewboard/diffviewer/renderers.py +++ b/reviewboard/diffviewer/renderers.py @@ -3,8 +3,9 @@ from django.http import HttpResponse from django.template.loader import render_to_string from django.utils.translation import gettext as _, get_language from djblets.cache.backend import cache_memoize +from djblets.deprecation import deprecate_non_keyword_only_args -from reviewboard.deprecation import RemovedInReviewBoard60Warning +from reviewboard.deprecation import RemovedInReviewBoard70Warning from reviewboard.diffviewer.chunk_generator import compute_chunk_last_header from reviewboard.diffviewer.diffutils import populate_diff_chunks from reviewboard.diffviewer.errors import UserVisibleError @@ -45,13 +46,25 @@ class DiffRenderer(object): #: reviewboard.diffviewer.settings.DiffSettings diff_settings: DiffSettings - def __init__(self, diff_file, chunk_index=None, highlighting=None, - collapse_all=True, lines_of_context=None, extra_context=None, - allow_caching=True, template_name=default_template_name, + @deprecate_non_keyword_only_args(RemovedInReviewBoard70Warning) + def __init__(self, + diff_file, + *, + chunk_index=None, + collapse_all=True, + lines_of_context=None, + extra_context=None, + allow_caching=True, + template_name=default_template_name, show_deleted=False, - *, diff_settings=None): + diff_settings): """Initialize the renderer. + Version Changed: + 6.0: + * Removed the ``highlighting`` argument. + * Made ``diff_settings`` mandatory. + Version Changed: 5.0.2: * Added ``diff_settings``, which will be required starting in @@ -65,10 +78,6 @@ class DiffRenderer(object): chunk_index (int, optional): The index of a specific chunk to render. - highlighting (bool, optional): - Whether to default to enabling syntax highlighting if - ``diff_settings`` is not provided. - Deprecated: 5.0.2: This has been replaced with ``diff_settings``. @@ -105,22 +114,6 @@ class DiffRenderer(object): Version Added: 5.0.2 """ - if highlighting is not None: - RemovedInReviewBoard60Warning.warn( - 'The `highlighting` argument to %r is deprecated and will ' - 'be removed in Review Board 6.0. Provide `diff_settings` ' - 'instead.' - % type(self)) - - if diff_settings is None: - diff_settings = DiffSettings.create() - - # Satisfy the type checker, due to the parameter being optional. - assert diff_settings is not None - - if highlighting is not None: - diff_settings.syntax_highlighting = highlighting - self.diff_file = diff_file self.diff_settings = diff_settings self.chunk_index = chunk_index diff --git a/reviewboard/diffviewer/tests/test_diff_chunk_generator.py b/reviewboard/diffviewer/tests/test_diff_chunk_generator.py index ac83a38482cd44ee30c23412c4ab8cf4e5c363b5..d507377e00b611ce939ba9d9f325ff153f3e9bc6 100644 --- a/reviewboard/diffviewer/tests/test_diff_chunk_generator.py +++ b/reviewboard/diffviewer/tests/test_diff_chunk_generator.py @@ -1,6 +1,7 @@ from kgb import SpyAgency from reviewboard.diffviewer.chunk_generator import DiffChunkGenerator +from reviewboard.diffviewer.settings import DiffSettings from reviewboard.scmtools.core import PRE_CREATION from reviewboard.testing import TestCase @@ -66,7 +67,8 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): self.repository = self.create_repository(tool_name='Test') self.diffset = self.create_diffset(repository=self.repository) self.filediff = self.create_filediff(diffset=self.diffset) - self.generator = DiffChunkGenerator(None, self.filediff) + self.generator = DiffChunkGenerator( + None, self.filediff, diff_settings=DiffSettings.create()) def test_get_chunks_with_empty_added_file(self): """Testing DiffChunkGenerator.get_chunks with empty added file""" @@ -179,7 +181,8 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): generator = DiffChunkGenerator(request=None, filediff=tip_filediff, - base_filediff=base_filediff) + base_filediff=base_filediff, + diff_settings=DiffSettings.create()) chunks = list(generator.get_chunks()) self.assertEqual(len(chunks), 1) @@ -225,7 +228,8 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): filediff = commit3.files.get() generator = DiffChunkGenerator(request=None, - filediff=filediff) + filediff=filediff, + diff_settings=DiffSettings.create()) chunks = list(generator.get_chunks()) self.assertEqual(len(chunks), 1) @@ -262,7 +266,8 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): generator = DiffChunkGenerator(request=None, filediff=filediff, - base_filediff=filediff) + base_filediff=filediff, + diff_settings=DiffSettings.create()) chunks = list(generator.get_chunks()) self.assertEqual(len(chunks), 1) @@ -287,7 +292,8 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): filediff = commit2.files.get() generator = DiffChunkGenerator(request=None, - filediff=filediff) + filediff=filediff, + diff_settings=DiffSettings.create()) chunks = list(generator.get_chunks()) self.assertEqual(len(chunks), 1) @@ -314,7 +320,8 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): generator = DiffChunkGenerator(request=None, filediff=filediff, - base_filediff=base_filediff) + base_filediff=base_filediff, + diff_settings=DiffSettings.create()) chunks = list(generator.get_chunks()) self.assertEqual(len(chunks), 1) @@ -350,10 +357,12 @@ class DiffChunkGeneratorTests(SpyAgency, TestCase): line_counts = self.filediff.get_line_counts() # Simulate an interdiff where the changes are reverted. - interdiff_generator = DiffChunkGenerator(request=None, - filediff=self.filediff, - interfilediff=None, - force_interdiff=True) + interdiff_generator = DiffChunkGenerator( + request=None, + filediff=self.filediff, + interfilediff=None, + force_interdiff=True, + diff_settings=DiffSettings.create()) # Again, just consuming the generator. self.assertEqual(len(list(interdiff_generator.get_chunks())), 1) diff --git a/reviewboard/diffviewer/tests/test_diff_parser.py b/reviewboard/diffviewer/tests/test_diff_parser.py index 7c681dc2faa44367dcf2a186c7b0383e9416ca95..2b0b543078623277a9f0db864604e82e9a9eef31 100644 --- a/reviewboard/diffviewer/tests/test_diff_parser.py +++ b/reviewboard/diffviewer/tests/test_diff_parser.py @@ -2,7 +2,6 @@ from djblets.testing.decorators import add_fixtures -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.diffviewer.testing.mixins import DiffParserTestingMixin from reviewboard.diffviewer.parser import (BaseDiffParser, DiffParser, @@ -52,42 +51,6 @@ class ParsedDiffFileTests(TestCase): self.assertEqual(parsed_diff_file.parent_parsed_diff_change, parsed_diff_change) - def test_init_with_parser(self): - """Testing ParsedDiffFile.__init__ with parser=""" - parser = BaseDiffParser(b'') - - message = ( - 'Diff parsers must pass a ParsedDiffChange as the ' - 'parsed_diff_change= parameter when creating a ParsedDiffFile. ' - 'They should no longer pass a parser= parameter. This will be ' - 'mandatory in Review Board 6.0.' - ) - - with self.assertWarns(cls=RemovedInReviewBoard60Warning, - message=message): - parsed_diff_file = ParsedDiffFile(parser=parser) - - self.assertIs(parsed_diff_file.parser, parser) - self.assertIsNone(parsed_diff_file.parent_parsed_diff_change) - - def test_init_with_no_parser_or_parsed_diff_change(self): - """Testing ParsedDiffFile.__init__ without parsed_diff_change= or - parser= - """ - message = ( - 'Diff parsers must pass a ParsedDiffChange as the ' - 'parsed_diff_change= parameter when creating a ParsedDiffFile. ' - 'They should no longer pass a parser= parameter. This will be ' - 'mandatory in Review Board 6.0.' - ) - - with self.assertWarns(cls=RemovedInReviewBoard60Warning, - message=message): - parsed_diff_file = ParsedDiffFile() - - self.assertIsNone(parsed_diff_file.parser) - self.assertIsNone(parsed_diff_file.parent_parsed_diff_change) - class DiffParserTest(DiffParserTestingMixin, TestCase): """Unit tests for reviewboard.diffviewer.parser.DiffParser.""" @@ -363,29 +326,3 @@ class DiffParserTest(DiffParserTestingMixin, TestCase): files = changes[0].files self.assertEqual(len(files), 1) self.assertEqual(files[0].extra_data, {'foo': True}) - - def test_parse_diff_with_get_orig_commit_id(self): - """Testing DiffParser.parse_diff with get_orig_commit_id() returning - a value - """ - class CustomParser(DiffParser): - def get_orig_commit_id(self): - return b'abc123' - - parser = CustomParser(self.DEFAULT_FILEDIFF_DATA_DIFF) - - message = ( - 'CustomParser.get_orig_commit_id() will no longer be supported ' - 'in Review Board 6.0. Please set the commit ID in ' - 'self.parsed_diff_change.parent_commit_id, and set ' - 'parsed_diff_change.uses_commit_ids_as_revisions = True.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - parsed_diff_file = parser.parse_diff() - - changes = parsed_diff_file.changes - self.assertEqual(len(changes), 1) - - self.assertTrue(parsed_diff_file.uses_commit_ids_as_revisions) - self.assertEqual(changes[0].parent_commit_id, b'abc123') diff --git a/reviewboard/diffviewer/tests/test_diff_renderer.py b/reviewboard/diffviewer/tests/test_diff_renderer.py index 0dc1514da7220ef87dc5895db791a84217fea527..874896bf31b52791f33090f7122b28e5f4fbf99a 100644 --- a/reviewboard/diffviewer/tests/test_diff_renderer.py +++ b/reviewboard/diffviewer/tests/test_diff_renderer.py @@ -3,7 +3,6 @@ from django.test import RequestFactory from djblets.cache.backend import cache_memoize from kgb import SpyAgency -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.diffviewer.errors import UserVisibleError from reviewboard.diffviewer.models import FileDiff from reviewboard.diffviewer.renderers import DiffRenderer @@ -24,11 +23,15 @@ class DiffRendererTests(SpyAgency, TestCase): 'chunks_loaded': True, } - renderer = DiffRenderer(diff_file, chunk_index=-1) + diff_settings = DiffSettings.create() + + renderer = DiffRenderer(diff_file, chunk_index=-1, + diff_settings=diff_settings) self.assertRaises(UserVisibleError, lambda: renderer.render_to_string_uncached(None)) - renderer = DiffRenderer(diff_file, chunk_index=1) + renderer = DiffRenderer(diff_file, chunk_index=1, + diff_settings=diff_settings) self.assertRaises(UserVisibleError, lambda: renderer.render_to_string_uncached(None)) @@ -40,7 +43,8 @@ class DiffRendererTests(SpyAgency, TestCase): } # Should not assert. - renderer = DiffRenderer(diff_file, chunk_index=0) + renderer = DiffRenderer(diff_file, chunk_index=0, + diff_settings=DiffSettings.create()) self.spy_on(renderer.render_to_string, call_original=False) self.spy_on(renderer.make_context, call_original=False) @@ -80,55 +84,13 @@ class DiffRendererTests(SpyAgency, TestCase): diff_settings=diff_settings) self.assertFalse(renderer.highlighting) - def test_construction_with_highlighting_true(self): - """Testing DiffRenderer construction with legacy highlighting=True""" - diff_file = { - 'chunks': [{}], - 'chunks_loaded': True, - } - - message = ( - "The `highlighting` argument to is deprecated " - "and will be removed in Review Board 6.0. Provide `diff_settings` " - "instead." - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - renderer = DiffRenderer(diff_file, - chunk_index=0, - highlighting=True) - - self.assertTrue(renderer.highlighting) - - def test_construction_with_highlighting_false(self): - """Testing DiffRenderer construction with legacy highlighting=False""" - diff_file = { - 'chunks': [{}], - 'chunks_loaded': True, - } - - message = ( - "The `highlighting` argument to is deprecated " - "and will be removed in Review Board 6.0. Provide `diff_settings` " - "instead." - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - renderer = DiffRenderer(diff_file, - chunk_index=0, - highlighting=False) - - self.assertFalse(renderer.highlighting) - def test_render_to_response(self): """Testing DiffRenderer.render_to_response""" diff_file = { 'chunks': [{}] } - renderer = DiffRenderer(diff_file) + renderer = DiffRenderer(diff_file, diff_settings=DiffSettings.create()) self.spy_on(renderer.render_to_string, call_fake=lambda self, request: 'Foo') @@ -146,7 +108,7 @@ class DiffRendererTests(SpyAgency, TestCase): 'chunks': [{}] } - renderer = DiffRenderer(diff_file) + renderer = DiffRenderer(diff_file, diff_settings=DiffSettings.create()) self.spy_on(renderer.render_to_string_uncached, call_fake=lambda self, request: 'Foo') self.spy_on(renderer.make_cache_key, @@ -168,7 +130,8 @@ class DiffRendererTests(SpyAgency, TestCase): 'chunks': [{}] } - renderer = DiffRenderer(diff_file, lines_of_context=[5, 5]) + renderer = DiffRenderer(diff_file, lines_of_context=[5, 5], + diff_settings=DiffSettings.create()) self.spy_on(renderer.render_to_string_uncached, call_fake=lambda self, request: 'Foo') self.spy_on(renderer.make_cache_key, @@ -211,7 +174,8 @@ class DiffRendererTests(SpyAgency, TestCase): ], } - renderer = DiffRenderer(diff_file, chunk_index=1) + renderer = DiffRenderer(diff_file, chunk_index=1, + diff_settings=DiffSettings.create()) context = renderer.make_context() self.assertEqual(context['standalone'], True) diff --git a/reviewboard/diffviewer/tests/test_diffutils.py b/reviewboard/diffviewer/tests/test_diffutils.py index 71d26db31bea0916ec7bc978426cac797fb1bc49..9c5ce0afb3fd7f0a389e6f50dc47072e3f709f36 100644 --- a/reviewboard/diffviewer/tests/test_diffutils.py +++ b/reviewboard/diffviewer/tests/test_diffutils.py @@ -26,6 +26,7 @@ from reviewboard.diffviewer.diffutils import ( _get_last_header_in_chunks_before_line) from reviewboard.diffviewer.errors import PatchError from reviewboard.diffviewer.models import DiffCommit, FileDiff +from reviewboard.diffviewer.settings import DiffSettings from reviewboard.scmtools.core import PRE_CREATION from reviewboard.scmtools.errors import FileNotFoundError from reviewboard.scmtools.models import Repository @@ -3122,10 +3123,12 @@ class DiffExpansionHeaderTests(TestCase): with self.siteconfig_settings(siteconfig_settings, reload_settings=False): + diff_settings = DiffSettings.create() header = get_last_header_before_line(context=context, filediff=filediff, interfilediff=None, - target_line=line_number) + target_line=line_number, + diff_settings=diff_settings) chunks = get_file_chunks_in_range( context=context, filediff=filediff, @@ -3134,7 +3137,9 @@ class DiffExpansionHeaderTests(TestCase): num_lines=get_last_line_number_in_diff( context=context, filediff=filediff, - interfilediff=None)) + interfilediff=None, + diff_settings=diff_settings), + diff_settings=diff_settings) lines = [] @@ -3190,10 +3195,12 @@ class DiffExpansionHeaderTests(TestCase): with self.siteconfig_settings(siteconfig_settings, reload_settings=False): + diff_settings = DiffSettings.create() header = get_last_header_before_line(context=context, filediff=filediff, interfilediff=None, - target_line=line_number) + target_line=line_number, + diff_settings=diff_settings) chunks = get_file_chunks_in_range( context=context, filediff=filediff, @@ -3202,7 +3209,9 @@ class DiffExpansionHeaderTests(TestCase): num_lines=get_last_line_number_in_diff( context=context, filediff=filediff, - interfilediff=None)) + interfilediff=None, + diff_settings=diff_settings), + diff_settings=diff_settings) lines = [] diff --git a/reviewboard/diffviewer/tests/test_forms.py b/reviewboard/diffviewer/tests/test_forms.py index 36c948f4cb9b9b2a40fc27c6a8e750cff1a20684..44ca6beb2affd865b57601d336cebed36d162ead 100644 --- a/reviewboard/diffviewer/tests/test_forms.py +++ b/reviewboard/diffviewer/tests/test_forms.py @@ -335,8 +335,8 @@ class UploadDiffFormTests(SpyAgency, TestCase): @unittest.skipIf(not is_exe_in_path('hg'), 'Hg is not installed') def test_create_with_parser_get_orig_commit_id(self): - """Testing UploadDiffForm.create uses correct base revision returned - by DiffParser.get_orig_commit_id + """Testing UploadDiffForm.create uses correct base revision set in + ParsedDiffChange.parent_commit_id """ diff = ( b'# Node ID a6fc203fee9091ff9739c9c00cd4a6694e023f48\n' diff --git a/reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py b/reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py index afd6c8ed789cf607057006ef3b9f8f791e0e23a8..579b53eeb666add66b3806c63ed2eaa74f2a470d 100644 --- a/reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py +++ b/reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py @@ -1,4 +1,3 @@ -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.diffviewer.chunk_generator import RawDiffChunkGenerator from reviewboard.diffviewer.settings import DiffSettings from reviewboard.testing import TestCase @@ -17,7 +16,8 @@ class RawDiffChunkGeneratorTests(TestCase): return RawDiffChunkGenerator(old=b'', new=b'', orig_filename='', - modified_filename='') + modified_filename='', + diff_settings=DiffSettings.create()) def test_get_chunks(self): """Testing RawDiffChunkGenerator.get_chunks""" @@ -34,7 +34,8 @@ class RawDiffChunkGeneratorTests(TestCase): b'la de doo.\n' ) - generator = RawDiffChunkGenerator(old, new, 'file1', 'file2') + generator = RawDiffChunkGenerator(old, new, 'file1', 'file2', + diff_settings=DiffSettings.create()) chunks = list(generator.get_chunks()) self.assertEqual(len(chunks), 4) @@ -238,116 +239,6 @@ class RawDiffChunkGeneratorTests(TestCase): } ) - def test_get_chunks_with_enable_syntax_highlighting_true(self): - """Testing RawDiffChunkGenerator.get_chunks with - legacy enable_syntax_highlighting=True and syntax highlighting - available for file - """ - old = b'This is **bold**' - new = b'This is *italic*' - - message = ( - "The `enable_syntax_highlighting` argument to " - "is deprecated and will be removed in Review Board 6.0. Please " - "provide `diff_settings` instead." - ) - - with self.assertWarns(cls=RemovedInReviewBoard60Warning, - message=message): - generator = RawDiffChunkGenerator(old=old, - new=new, - orig_filename='file1.md', - modified_filename='file2.md', - enable_syntax_highlighting=True) - - self.assertTrue(generator.enable_syntax_highlighting) - - chunks = list(generator.get_chunks()) - - self.assertEqual(len(chunks), 1) - self.assertEqual( - chunks[0], - { - 'change': 'replace', - 'collapsable': False, - 'index': 0, - 'lines': [ - [ - 1, - 1, - 'This is **bold**', - [(9, 16)], - 1, - 'This is *italic*', - [(9, 16)], - False, - ], - ], - 'meta': { - 'left_headers': [], - 'right_headers': [], - 'whitespace_chunk': False, - 'whitespace_lines': [], - }, - 'numlines': 1, - } - ) - - def test_get_chunks_with_enable_syntax_highlighting_false(self): - """Testing RawDiffChunkGenerator.get_chunks with - legacy enable_syntax_highlighting=False - """ - old = b'This is **bold**' - new = b'This is *italic*' - - message = ( - "The `enable_syntax_highlighting` argument to " - "is deprecated and will be removed in Review Board 6.0. Please " - "provide `diff_settings` instead." - ) - - with self.assertWarns(cls=RemovedInReviewBoard60Warning, - message=message): - generator = RawDiffChunkGenerator(old=old, - new=new, - orig_filename='file1.md', - modified_filename='file2.md', - enable_syntax_highlighting=False) - - self.assertFalse(generator.enable_syntax_highlighting) - - chunks = list(generator.get_chunks()) - - self.assertEqual(len(chunks), 1) - self.assertEqual( - chunks[0], - { - 'change': 'replace', - 'collapsable': False, - 'index': 0, - 'lines': [ - [ - 1, - 1, - 'This is **bold**', - [(9, 16)], - 1, - 'This is *italic*', - [(9, 16)], - False, - ], - ], - 'meta': { - 'left_headers': [], - 'right_headers': [], - 'whitespace_chunk': False, - 'whitespace_lines': [], - }, - 'numlines': 1, - } - ) def test_get_chunks_with_syntax_highlighting_blacklisted(self): """Testing RawDiffChunkGenerator.get_chunks with syntax highlighting @@ -419,7 +310,8 @@ class RawDiffChunkGeneratorTests(TestCase): generator = RawDiffChunkGenerator(old=old, new=new, orig_filename='file1', - modified_filename='file2') + modified_filename='file2', + diff_settings=DiffSettings.create()) chunks = list(generator.generate_chunks( old=old, new=new, @@ -542,7 +434,8 @@ class RawDiffChunkGeneratorTests(TestCase): old=old, new=new, orig_filename='file1.c', - modified_filename='file2.py') + modified_filename='file2.py', + diff_settings=DiffSettings.create()) chunks = list(chunk_generator.get_chunks()) self.assertEqual(len(chunks), 1) @@ -599,10 +492,12 @@ class RawDiffChunkGeneratorTests(TestCase): def test_apply_pygments_with_lexer(self): """Testing RawDiffChunkGenerator._apply_pygments with valid lexer""" - chunk_generator = RawDiffChunkGenerator(old=[], - new=[], - orig_filename='file1', - modified_filename='file2') + chunk_generator = RawDiffChunkGenerator( + old=[], + new=[], + orig_filename='file1', + modified_filename='file2', + diff_settings=DiffSettings.create()) self.assertEqual( chunk_generator._apply_pygments(data='This is **bold**\n', filename='test.md'), @@ -610,10 +505,12 @@ class RawDiffChunkGeneratorTests(TestCase): def test_apply_pygments_without_lexer(self): """Testing RawDiffChunkGenerator._apply_pygments without valid lexer""" - chunk_generator = RawDiffChunkGenerator(old=[], - new=[], - orig_filename='file1', - modified_filename='file2') + chunk_generator = RawDiffChunkGenerator( + old=[], + new=[], + orig_filename='file1', + modified_filename='file2', + diff_settings=DiffSettings.create()) self.assertIsNone( chunk_generator._apply_pygments(data='This is **bold**', filename='test')) @@ -627,10 +524,12 @@ class RawDiffChunkGeneratorTests(TestCase): '.md', ) - chunk_generator = MyRawDiffChunkGenerator(old=[], - new=[], - orig_filename='file1', - modified_filename='file2') + chunk_generator = MyRawDiffChunkGenerator( + old=[], + new=[], + orig_filename='file1', + modified_filename='file2', + diff_settings=DiffSettings.create()) self.assertIsNone( chunk_generator._apply_pygments(data='This is **bold**', filename='test.md')) @@ -644,10 +543,12 @@ class RawDiffChunkGeneratorTests(TestCase): } with self.siteconfig_settings(settings): - chunk_generator = RawDiffChunkGenerator(old=[], - new=[], - orig_filename='file1', - modified_filename='file2') + chunk_generator = RawDiffChunkGenerator( + old=[], + new=[], + orig_filename='file1', + modified_filename='file2', + diff_settings=DiffSettings.create()) data = 'This is **bold**' correct_output = ( 'This ' @@ -669,10 +570,12 @@ class RawDiffChunkGeneratorTests(TestCase): } with self.siteconfig_settings(settings): - chunk_generator = RawDiffChunkGenerator(old=[], - new=[], - orig_filename='file1', - modified_filename='file2') + chunk_generator = RawDiffChunkGenerator( + old=[], + new=[], + orig_filename='file1', + modified_filename='file2', + diff_settings=DiffSettings.create()) self.assertEqual( chunk_generator._apply_pygments(data='This is **bold**\n', filename='test.md'), @@ -687,10 +590,12 @@ class RawDiffChunkGeneratorTests(TestCase): } with self.siteconfig_settings(settings): - chunk_generator = RawDiffChunkGenerator(old=[], - new=[], - orig_filename='file1', - modified_filename='file2') + chunk_generator = RawDiffChunkGenerator( + old=[], + new=[], + orig_filename='file1', + modified_filename='file2', + diff_settings=DiffSettings.create()) data = '.test() { .a { margin: 0; } }' expected_log_output = ( 'Pygments lexer "NonExistantClass" for ".less" files in ' @@ -707,7 +612,8 @@ class RawDiffChunkGeneratorTests(TestCase): """Testing RawDiffChunkGenerator._get_move_info with new move range and no adjacent preceding move range """ - generator = RawDiffChunkGenerator([], [], 'file1', 'file2') + generator = RawDiffChunkGenerator([], [], 'file1', 'file2', + diff_settings=DiffSettings.create()) self.assertEqual( generator._get_move_info(10, { @@ -721,7 +627,8 @@ class RawDiffChunkGeneratorTests(TestCase): """Testing RawDiffChunkGenerator._get_move_info with new move range and adjacent preceding move range """ - generator = RawDiffChunkGenerator([], [], 'file1', 'file2') + generator = RawDiffChunkGenerator([], [], 'file1', 'file2', + diff_settings=DiffSettings.create()) self.assertEqual( generator._get_move_info(10, { @@ -736,7 +643,8 @@ class RawDiffChunkGeneratorTests(TestCase): """Testing RawDiffChunkGenerator._get_move_info with existing move range """ - generator = RawDiffChunkGenerator([], [], 'file1', 'file2') + generator = RawDiffChunkGenerator([], [], 'file1', 'file2', + diff_settings=DiffSettings.create()) self.assertEqual( generator._get_move_info(11, { @@ -749,7 +657,8 @@ class RawDiffChunkGeneratorTests(TestCase): def test_get_move_info_with_no_move(self): """Testing RawDiffChunkGenerator._get_move_info with no move range""" - generator = RawDiffChunkGenerator([], [], 'file1', 'file2') + generator = RawDiffChunkGenerator([], [], 'file1', 'file2', + diff_settings=DiffSettings.create()) self.assertIsNone(generator._get_move_info(500, { 8: 100, diff --git a/reviewboard/hostingsvcs/managers.py b/reviewboard/hostingsvcs/managers.py index a5874be969b0f1ef2b068e1da24ed831fc224391..ece9667b5062c12f098eb6277b8b63991d99f7ba 100644 --- a/reviewboard/hostingsvcs/managers.py +++ b/reviewboard/hostingsvcs/managers.py @@ -1,19 +1,21 @@ from django.db.models import Manager, Q -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.site.models import LocalSite class HostingServiceAccountManager(Manager): """A manager for HostingServiceAccount models.""" - def accessible(self, visible_only=True, local_site=None, - filter_local_site=None): + def accessible(self, visible_only=True, local_site=None): """Return hosting service accounts that are accessible. These will include all visible accounts that are compatible with the specified :term:`Local Site`. + Version Changed: + 6.0: + Removed the ``filter_local_site`` argument. + Version Changed: 5.0: Deprecated ``filter_local_site`` and added support for @@ -38,33 +40,10 @@ class HostingServiceAccountManager(Manager): Added support for :py:attr:`LocalSite.ALL `. - filter_local_site (bool, optional): - Whether to factor in the ``local_site`` argument. If ``False``, - the :term:`Local Site` will be ignored. - - Deprecated: - 5.0: - Callers should instead set ``local_site`` to - :py:class:`LocalSite.ALL - ` instead of - setting this to ``True``. - Returns: django.db.models.query.QuerySet: The resulting queryset. """ - if filter_local_site is not None: - RemovedInReviewBoard60Warning.warn( - 'filter_local_site is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.') - - if filter_local_site: - assert local_site is not LocalSite.ALL - else: - assert local_site in (None, LocalSite.ALL) - local_site = LocalSite.ALL - q = LocalSite.objects.build_q(local_site=local_site) if visible_only: diff --git a/reviewboard/integrations/__init__.py b/reviewboard/integrations/__init__.py index 03efbbed9aaba9e7aa162fbb40894046a6fe3031..e93e4543f3388353182762f39ef3e49b9e2078aa 100644 --- a/reviewboard/integrations/__init__.py +++ b/reviewboard/integrations/__init__.py @@ -4,93 +4,4 @@ This module provides the Review Board functionality needed to create integrations for third-party services. It builds upon Djblets's integrations foundation, offering some additional utilities for more easily creating manageable integrations. - -The functions and classes used in this module are deprecated. Consumers should -use the versions in :py:mod:`reviewboard.integrations.base` instead. """ - -from reviewboard.deprecation import RemovedInReviewBoard60Warning - - -def get_integration_manager(): - """Return the integrations manager. - - Deprecated: - 4.0: - This has been deprecated in favor of - :py:func:`reviewboard.integrations.base.get_integration_manager`. - - Returns: - djblets.integrations.manager.IntegrationManager: - The Review Board integrations manager. - """ - from reviewboard.integrations.base import (get_integration_manager as - _get_integration_manager) - - RemovedInReviewBoard60Warning.warn( - 'reviewboard.integrations.get_integration_manager() is deprecated and ' - 'will be removed in Review Board 6.0. Use reviewboard.integrations.' - 'base.get_integration_manager() instead.') - - return _get_integration_manager() - - -class _ProxyIntegrationMetaClass(type): - """Metaclass for a deprecated forwarding Integration class. - - This is used along with :py:class:`Integration` to allow older code - that subclasses :py:class:`reviewboard.integrations.Integration` to - instead automatically subclass - :py:class:`reviewboard.integrations.base.Integration`, emitting a warning - in the process to notify authors to update their code. - """ - - def __new__(cls, name, bases, d): - """Create the subclass of an integration. - - Args: - name (str): - The name of the integration subclass. - - bases (tuple): - The parent classes. - - d (dict): - The class dictionary. - - Returns: - type: - The new class. - """ - if bases != (object,): - # This is a subclass of Integration. - from reviewboard.integrations.base import (Integration as - BaseIntegration) - - RemovedInReviewBoard60Warning.warn( - 'reviewboard.integrations.Integration is deprecated and will ' - 'be removed in Review Board 6.0. %s should inherit from ' - 'reviewboard.integrations.base.Integration instead.' - % name) - - new_bases = [] - - for base in bases: - if base is Integration: - new_bases.append(BaseIntegration) - else: - new_bases.append(base) - - bases = tuple(new_bases) - - return type.__new__(cls, name, bases, d) - - -class Integration(object, metaclass=_ProxyIntegrationMetaClass): - """Base class for an integration. - - Deprecated: - 4.0: - Subclasses should inherit from - :py:class:`reviewboard.integrations.base.Integration` instead. - """ diff --git a/reviewboard/reviews/managers.py b/reviewboard/reviews/managers.py index 37718197cd2e2a7905df1bf83bdfe1a2cb50e9f1..9ceb318a3c834eb9469e09e10efd92f6a79b5849 100644 --- a/reviewboard/reviews/managers.py +++ b/reviewboard/reviews/managers.py @@ -11,9 +11,10 @@ from django.db import connections, router, transaction, IntegrityError from django.db.models import Manager, Q from django.db.models.query import QuerySet from django.utils.text import slugify +from djblets.deprecation import deprecate_non_keyword_only_args from djblets.db.managers import ConcurrencyManager -from reviewboard.deprecation import RemovedInReviewBoard60Warning +from reviewboard.deprecation import RemovedInReviewBoard70Warning from reviewboard.diffviewer.models import DiffSetHistory from reviewboard.scmtools.errors import ChangeNumberInUseError from reviewboard.scmtools.models import Repository @@ -254,8 +255,13 @@ class DefaultReviewerManager(Manager): class ReviewGroupManager(Manager): """A manager for Group models.""" - def accessible(self, user, visible_only=True, local_site=None, - show_all_local_sites=None, distinct=True): + @deprecate_non_keyword_only_args(RemovedInReviewBoard70Warning) + def accessible(self, + user, + *, + visible_only=True, + local_site=None, + distinct=True): """Return a queryset for review groups accessible by the given user. For superusers, all public and invite-only review groups will be @@ -269,6 +275,10 @@ class ReviewGroupManager(Manager): The returned list is further filtered down based on the ``visible_only`` and ``local_site`` parameters. + Version Changed: + 6.0: + Removed the ``show_all_local_sites`` argument. + Version Changed: 5.0: Deprecated ``show_all_local_sites`` and added support for @@ -300,18 +310,6 @@ class ReviewGroupManager(Manager): Added support for :py:attr:`LocalSite.ALL `. - show_all_local_sites (bool, optional): - Whether review groups for all :term:`Local Sites` should be - returned. This cannot be ``True`` if a ``local_site`` - instance was provided. - - Deprecated: - 5.0: - Callers should instead set ``local_site`` to - :py:class:`LocalSite.ALL - ` instead of - setting this to ``True``. - distinct (bool, optional): Whether to return distinct results. @@ -322,18 +320,6 @@ class ReviewGroupManager(Manager): django.db.models.query.QuerySet: The resulting queryset. """ - if show_all_local_sites is not None: - RemovedInReviewBoard60Warning.warn( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.') - - if show_all_local_sites: - assert local_site in (None, LocalSite.ALL) - local_site = LocalSite.ALL - else: - assert local_site is not LocalSite.ALL - q = Q() if user.is_superuser: @@ -1039,8 +1025,7 @@ class ReviewRequestManager(ConcurrencyManager): def _query(self, user=None, status='P', with_counts=False, extra_query=None, local_site=None, filter_private=False, - show_inactive=False, show_all_unpublished=False, - show_all_local_sites=None): + show_inactive=False, show_all_unpublished=False): """Return a queryset for review requests matching the given criteria. By default, the results will not be filtered based on whether a user @@ -1048,6 +1033,10 @@ class ReviewRequestManager(ConcurrencyManager): invite-only review group ACLs). To filter based on access, pass ``filter_private=True``. + Version Changed: + 6.0: + Removed the ``show_all_local_sites`` argument. + Version Changed: 5.0: Deprecated ``show_all_local_sites`` and added support for @@ -1102,36 +1091,12 @@ class ReviewRequestManager(ConcurrencyManager): show_all_unpublished (bool, optional): Whether to include review requests not yet published. - show_all_local_sites (bool, optional): - Whether review requests for all :term:`Local Sites` should be - returned. This cannot be ``True`` if a ``local_site`` - instance was provided. - - Deprecated: - 5.0: - Callers should instead set ``local_site`` to - :py:class:`LocalSite.ALL - ` instead of - setting this to ``True``. - Returns: django.db.models.query.QuerySet: The resulting queryset. """ from reviewboard.reviews.models import Group - if show_all_local_sites is not None: - RemovedInReviewBoard60Warning.warn( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.') - - if show_all_local_sites: - assert local_site in (None, LocalSite.ALL) - local_site = LocalSite.ALL - else: - assert local_site is not LocalSite.ALL - is_authenticated = (user is not None and user.is_authenticated) if show_all_unpublished: diff --git a/reviewboard/reviews/tests/test_review_group_manager.py b/reviewboard/reviews/tests/test_review_group_manager.py index 0e6ce11a15ed91a7d305d7e9f90d857a1af80a65..3294d5776e58f071e249bef72e8eb294cc1b32c2 100644 --- a/reviewboard/reviews/tests/test_review_group_manager.py +++ b/reviewboard/reviews/tests/test_review_group_manager.py @@ -3,7 +3,6 @@ from django.contrib.auth.models import AnonymousUser from djblets.testing.decorators import add_fixtures -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.reviews.models import Group from reviewboard.site.models import LocalSite from reviewboard.testing import TestCase @@ -184,17 +183,6 @@ class ReviewGroupManagerTests(TestCase): group, Group.objects.accessible(user, local_site=LocalSite.ALL)) - message = ( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - self.assertIn( - group, - Group.objects.accessible(user, show_all_local_sites=True)) - def test_accessible_ids_with_public(self): """Testing Group.objects.accessible_ids with public group""" anonymous = AnonymousUser() @@ -367,14 +355,3 @@ class ReviewGroupManagerTests(TestCase): self.assertIn( group.pk, Group.objects.accessible_ids(user, local_site=LocalSite.ALL)) - - message = ( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - self.assertIn( - group.pk, - Group.objects.accessible_ids(user, show_all_local_sites=True)) diff --git a/reviewboard/reviews/ui/text.py b/reviewboard/reviews/ui/text.py index d937d1ddfe2c3406d9c610dea90c23e76cacaa28..3ab00e2dd15ea96884bf68ebeb056ae54d3bef17 100644 --- a/reviewboard/reviews/ui/text.py +++ b/reviewboard/reviews/ui/text.py @@ -13,6 +13,7 @@ from reviewboard.attachments.models import FileAttachment from reviewboard.diffviewer.chunk_generator import (NoWrapperHtmlFormatter, RawDiffChunkGenerator) from reviewboard.diffviewer.diffutils import get_chunks_in_range +from reviewboard.diffviewer.settings import DiffSettings from reviewboard.reviews.ui.base import FileAttachmentReviewUI @@ -339,7 +340,8 @@ class TextBasedReviewUI(FileAttachmentReviewUI): old=orig, new=modified, orig_filename=self.obj.filename, - modified_filename=self.diff_against_obj.filename) + modified_filename=self.diff_against_obj.filename, + diff_settings=DiffSettings.create(request=self.request)) def _get_source_diff_chunk_generator(self): """Return a chunk generator for diffing source text. diff --git a/reviewboard/scmtools/forms.py b/reviewboard/scmtools/forms.py index 564b63a6f941941de5eda6788030a2f206fa7592..9facd3da05490fe17ee2ba9e2ba1fc78a857587b 100644 --- a/reviewboard/scmtools/forms.py +++ b/reviewboard/scmtools/forms.py @@ -19,7 +19,6 @@ from reviewboard.admin.form_widgets import (RelatedGroupWidget, RelatedUserWidget) from reviewboard.admin.import_utils import has_module from reviewboard.admin.validation import validate_bug_tracker -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.hostingsvcs.errors import (AuthorizationError, HostingServiceError, SSHKeyAssociationError, @@ -2285,22 +2284,10 @@ class RepositoryForm(LocalSiteAwareModelFormMixin, forms.ModelForm): plan=plan, **repository_extra_data) else: - if func_accepts_kwargs(scmtool.check_repository): - scmtool.check_repository( - local_site=self.local_site, - local_site_name=local_site_name, - **repository_extra_data) - else: - RemovedInReviewBoard60Warning.warn( - '%s.check_repository must accept **kwargs. ' - 'It should also make sure it accepts "path", ' - '"username", "password", and "local_site_name" ' - 'as keyword arguments in any order. This will be ' - 'required in Review Board 6.0.' - % scmtool.__name__) - - scmtool.check_repository( - path, username, password, local_site_name) + scmtool.check_repository( + local_site=self.local_site, + local_site_name=local_site_name, + **repository_extra_data) # Success. break diff --git a/reviewboard/scmtools/hg.py b/reviewboard/scmtools/hg.py index 6d6845b2ee8949a60693ee765a26a1bb51423553..e0cff145bd749e916a0eb857c9f30682c9a89158 100644 --- a/reviewboard/scmtools/hg.py +++ b/reviewboard/scmtools/hg.py @@ -286,20 +286,21 @@ class HgTool(SCMTool): class HgDiffParser(DiffParser): """Diff parser for native Mercurial diffs.""" - def __init__(self, data): + def __init__(self, data, **kwargs): """Initialize the parser. Args: data (bytes): The diff content to parse. + **kwargs (dict): + Keyword arguments to pass to the parent class. + Raises: TypeError: The provided ``data`` argument was not a ``bytes`` type. """ - super(HgDiffParser, self).__init__(data) - - self.orig_changeset_id = None + super().__init__(data, uses_commit_ids_as_revisions=True) def parse_special_header(self, linenum, parsed_file): """Parse a special diff header marking the start of a new file's info. @@ -334,7 +335,7 @@ class HgDiffParser(DiffParser): split_line = diff_line.split() if diff_line.startswith(b'# Parent') and len(split_line) == 3: - self.orig_changeset_id = split_line[2] + self.parsed_diff_change.parent_commit_id = split_line[2] elif diff_line.startswith(b'diff -r'): # A diff between two revisions are in the following form: # @@ -361,7 +362,7 @@ class HgDiffParser(DiffParser): parsed_file.orig_file_details = split_line[2] parsed_file.modified_filename = filename - self.orig_changeset_id = split_line[2] + self.parsed_diff_change.parent_commit_id = split_line[2] except ValueError: raise DiffParserError('The diff file is missing revision ' 'information', @@ -411,20 +412,6 @@ class HgDiffParser(DiffParser): return linenum - def get_orig_commit_id(self): - """Return the commit ID of the original revision for the diff. - - This returns the commit ID found in a previously-parsed ``# Parent`` - line. It will override the values being stored in - :py:attr:`FileDiff.source_revision - `. - - Returns: - bytes: - The commit ID to return. - """ - return self.orig_changeset_id - class HgGitDiffParser(GitDiffParser): """Diff Parser for Git diffs generated by Mercurial. @@ -433,6 +420,22 @@ class HgGitDiffParser(GitDiffParser): be parsed in order to properly locate changes to files in a repository. """ + def __init__(self, data, **kwargs): + """Initialize the parser. + + Args: + data (bytes): + The diff content to parse. + + **kwargs (dict): + Keyword arguments to pass to the parent class. + + Raises: + TypeError: + The provided ``data`` argument was not a ``bytes`` type. + """ + super().__init__(data, uses_commit_ids_as_revisions=True) + def parse(self): """Parse the diff. @@ -465,23 +468,10 @@ class HgGitDiffParser(GitDiffParser): self.new_commit_id = split_line[3] elif line.startswith(b'# Parent') and len(split_line) == 3: self.base_commit_id = split_line[2] + self.parsed_diff_change.parent_commit_id = split_line[2] return super(HgGitDiffParser, self).parse() - def get_orig_commit_id(self): - """Return the commit ID of the original revision for the diff. - - This returns the commit ID found in a previously-parsed ``# Parent`` - line. It will override the values being stored in - :py:attr:`FileDiff.source_revision - `. - - Returns: - bytes: - The commit ID to return. - """ - return self.base_commit_id - class HgWebClient(SCMClient): FULL_FILE_URL = '%(url)s/%(rawpath)s/%(revision)s/%(quoted_path)s' diff --git a/reviewboard/scmtools/managers.py b/reviewboard/scmtools/managers.py index 38d52ef02ab845a9f2fa9c0a48f43e51375557c4..9174168459f334b7f2da8c4c826c9f6ddd4e3972 100644 --- a/reviewboard/scmtools/managers.py +++ b/reviewboard/scmtools/managers.py @@ -3,8 +3,9 @@ import logging import pkg_resources from django.db.models import Manager, Q from django.db.models.query import QuerySet +from djblets.deprecation import deprecate_non_keyword_only_args -from reviewboard.deprecation import RemovedInReviewBoard60Warning +from reviewboard.deprecation import RemovedInReviewBoard70Warning from reviewboard.site.models import LocalSite @@ -163,8 +164,14 @@ class ToolManager(Manager): class RepositoryManager(Manager): """A manager for Repository models.""" - def accessible(self, user, visible_only=True, local_site=None, - show_all_local_sites=None, distinct=True): + @deprecate_non_keyword_only_args(RemovedInReviewBoard70Warning) + def accessible( + self, + user, + *, + visible_only=True, + local_site=None, + distinct=True): """Return a queryset for repositories accessible by the given user. For superusers, all public and private repositories will be returned. @@ -178,6 +185,10 @@ class RepositoryManager(Manager): The returned list is further filtered down based on the ``visible_only`` and ``local_site`` parameters. + Version Changed: + 6.0: + Removed the ``show_all_local_sites`` argument. + Version Changed: 5.0: Deprecated ``show_all_local_sites`` and added support for @@ -209,18 +220,6 @@ class RepositoryManager(Manager): Added support for :py:attr:`LocalSite.ALL `. - show_all_local_sites (bool, optional): - Whether repositories from all :term:`Local Sites` should be - returned. This cannot be ``True`` if a ``local_site`` instance - was provided. - - Deprecated: - 5.0: - Callers should instead set ``local_site`` to - :py:class:`LocalSite.ALL - ` instead of - setting this to ``True``. - distinct (bool, optional): Whether to return distinct results. @@ -231,18 +230,6 @@ class RepositoryManager(Manager): django.db.models.query.QuerySet: The resulting queryset. """ - if show_all_local_sites is not None: - RemovedInReviewBoard60Warning.warn( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.') - - if show_all_local_sites: - assert local_site in (None, LocalSite.ALL) - local_site = LocalSite.ALL - else: - assert local_site is not LocalSite.ALL - q = Q() if user.is_superuser: diff --git a/reviewboard/scmtools/models.py b/reviewboard/scmtools/models.py index 0ae8405ed8dd74e0cd55e573753717e27283e6fc..6ebb383f0b0584f31d0817a4af455e8d216d24be 100644 --- a/reviewboard/scmtools/models.py +++ b/reviewboard/scmtools/models.py @@ -17,7 +17,6 @@ from djblets.db.fields import JSONField from djblets.log import log_timed from djblets.util.decorators import cached_property -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.hostingsvcs.errors import MissingHostingServiceError from reviewboard.hostingsvcs.models import HostingServiceAccount from reviewboard.hostingsvcs.service import get_hosting_service @@ -35,35 +34,6 @@ from reviewboard.site.models import LocalSite logger = logging.getLogger(__name__) -def _deprecated_proxy_property(property_name): - """Implement a proxy property for tools. - - The Tool class includes a number of properties that are proxied from the - scmtool class, and we'd like to transition away from them in favor of - having users get that information directly from the scmtool instead. - - Version Added: - 5.0 - - Args: - property_name (str): - The name of the property to proxy. - - Returns: - property: - A property that will proxy the value and raise a warning. - """ - @wraps(property_name) - def _wrapped(tool): - RemovedInReviewBoard60Warning.warn( - 'The Tool.%s property is deprecated and will be removed in ' - 'Review Board 6.0. Use the property on the repository or ' - 'SCMTool class instead.' - % property_name) - return getattr(tool.scmtool_class, property_name) - return property(_wrapped) - - class Tool(models.Model): """A configured source code management tool. @@ -87,33 +57,6 @@ class Tool(models.Model): # instantiate the class, which will fail without the necessary parameters. # So, we use these as convenient wrappers to do what the template can't do. - #: Whether or not the SCMTool supports review requests with history. - #: - #: See :py:attr:`SCMTool.supports_history - #: ` for details. - supports_history = _deprecated_proxy_property('supports_history') - - #: Whether custom URL masks can be defined to fetching file contents. - #: - #: See :py:attr:`SCMTool.supports_raw_file_urls - #: ` for details. - supports_raw_file_urls = _deprecated_proxy_property( - 'supports_raw_file_urls') - - #: Whether ticket-based authentication is supported. - #: - #: See :py:attr:`SCMTool.supports_ticket_auth - #: ` for details. - supports_ticket_auth = _deprecated_proxy_property('supports_ticket_auth') - - #: Whether server-side pending changesets are supported. - #: - #: See :py:attr:`SCMTool.supports_pending_changesets - #: ` for - #: details. - supports_pending_changesets = _deprecated_proxy_property( - 'supports_pending_changesets') - #: Overridden help text for the configuration form fields. #: #: See :py:attr:`SCMTool.field_help_text diff --git a/reviewboard/scmtools/registry.py b/reviewboard/scmtools/registry.py index 5636d7fae8f55499c554266e84761943af3db532..65bfcbd3bdd07c45d1f6665ff00f42702f85d982 100644 --- a/reviewboard/scmtools/registry.py +++ b/reviewboard/scmtools/registry.py @@ -13,7 +13,6 @@ from django.utils.translation import gettext_lazy as _ from djblets.registries.registry import (ALREADY_REGISTERED, LOAD_ENTRY_POINT, NOT_REGISTERED) -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.registries.registry import EntryPointRegistry from reviewboard.scmtools.models import Tool @@ -138,26 +137,6 @@ class SCMToolRegistry(EntryPointRegistry): if new_tools: Tool.objects.bulk_create(new_tools) - # Look to see if anything exists in the database but does not exist - # in entry points. - for tool in tools: - if self.get_by_class_name(tool.class_name) is None: - try: - self.register(tool.get_scmtool_class()) - RemovedInReviewBoard60Warning.warn( - 'SCMTool %s was found in the Tool table in the ' - 'database, but not in an entry point. The Tool ' - 'table will be removed in Review Board 6.0. To ' - 'continue using this tool, it must be manually ' - 'added by calling the register() method on the ' - 'SCMTools registry.' - % tool.class_name) - except ImproperlyConfigured as e: - logger.warning( - 'SCMTool %r in the scmtools_tool table could not be ' - 'loaded: %s' - % (tool.class_name, e)) - def get_defaults(self): """Yield to built-in SCMTools. diff --git a/reviewboard/scmtools/tests/test_registry.py b/reviewboard/scmtools/tests/test_registry.py index 4883838d7936bddf6bf20e5ba64db73aceff2250..25c9da740f32c9f3c84859bccd7bd2f1383749cc 100644 --- a/reviewboard/scmtools/tests/test_registry.py +++ b/reviewboard/scmtools/tests/test_registry.py @@ -68,7 +68,7 @@ class SCMToolRegistryTests(TestCase): with self.assertLogs(logger=logger) as log_ctx: scmtools_registry.populate_db() - self.assertEqual(len(log_ctx.records), 3) + self.assertEqual(len(log_ctx.records), 2) self.assertEqual( log_ctx.records[0].getMessage(), "Tool ID %s (name='Git!', " @@ -83,9 +83,3 @@ class SCMToolRegistryTests(TestCase): "conflicts with SCMTool 'perforce' (name='Perforce', " "class_name='reviewboard.scmtools.perforce.PerforceTool')" % perforce.pk) - self.assertEqual( - log_ctx.records[2].getMessage(), - "SCMTool 'reviewboard.scmtools.XXXOtherPerforceTool' in the " - "scmtools_tool table could not be loaded: Module " - "\"reviewboard.scmtools\" does not define a " - "\"XXXOtherPerforceTool\" SCM Tool") diff --git a/reviewboard/scmtools/tests/test_repository_form.py b/reviewboard/scmtools/tests/test_repository_form.py index a132829815f2e6065333adc917217ad0261fec53..29f83c9d8d480deda955b913e009fcf69eeca096 100644 --- a/reviewboard/scmtools/tests/test_repository_form.py +++ b/reviewboard/scmtools/tests/test_repository_form.py @@ -7,7 +7,6 @@ from django.contrib.auth.models import User from django.http import QueryDict from kgb import SpyAgency -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.hostingsvcs.models import HostingServiceAccount from reviewboard.hostingsvcs.github import GitHub from reviewboard.hostingsvcs.service import (get_hosting_service, @@ -947,99 +946,6 @@ class RepositoryFormTests(SpyAgency, TestCase): raw_file_url='https://git.example.com/raw///', username='myuser') - def test_plain_repository_with_deprecated_check_repository(self): - """Testing RepositoryForm with a plain repository and deprecated - check_repository() - """ - class CustomAuthForm(StandardSCMToolAuthForm): - custom_field1 = forms.CharField() - custom_field2 = forms.BooleanField() - - class CustomRepositoryForm(StandardSCMToolRepositoryForm): - custom_field3 = forms.CharField() - custom_field4 = forms.IntegerField() - - @classmethod - def _check_repository(cls, path, username=None, password=None, - local_site_name=None): - pass - - old_auth_form = GitTool.auth_form - old_repository_form = GitTool.repository_form - old_check_repository = GitTool.check_repository - - GitTool.auth_form = CustomAuthForm - GitTool.repository_form = CustomRepositoryForm - GitTool.check_repository = _check_repository - - try: - form = self._build_form({ - 'name': 'test', - 'tool': 'git', - 'path': '/path/to/test.git', - 'mirror_path': 'git@localhost:test.git', - 'raw_file_url': - 'https://git.example.com/raw///', - 'username': 'myuser', - 'password': 'mypass', - 'git-custom_field1': 'str1', - 'git-custom_field2': 'true', - 'git-custom_field3': 'str2', - 'git-custom_field4': '123', - }) - - message = ( - 'GitTool.check_repository must accept **kwargs. It should ' - 'also make sure it accepts "path", "username", "password", ' - 'and "local_site_name" as keyword arguments in any order. ' - 'This will be required in Review Board 6.0.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - self.assertTrue(form.is_valid()) - repository = form.save() - - self.assertEqual(repository.name, 'test') - self.assertEqual(repository.tool, Tool.objects.get(name='Git')) - self.assertEqual(repository.hosting_account, None) - self.assertEqual(repository.path, '/path/to/test.git') - self.assertEqual(repository.mirror_path, 'git@localhost:test.git') - self.assertEqual( - repository.raw_file_url, - 'https://git.example.com/raw///') - self.assertEqual(repository.extra_data, { - 'git-custom_field1': 'str1', - 'git-custom_field2': True, - 'git-custom_field3': 'str2', - 'git-custom_field4': 123, - }) - self.assertEqual(repository.get_credentials(), { - 'username': 'myuser', - 'password': 'mypass', - }) - self.assertEqual( - list(form.iter_subforms(bound_only=True)), - [ - form.scmtool_repository_forms['git'], - ]) - - # Make sure none of the other auth forms are unhappy. That would - # be an indicator that we're doing form processing and validation - # wrong. - for auth_form in form.hosting_auth_forms.values(): - self.assertEqual(auth_form.errors, {}) - - self.assertSpyCalledWith( - GitTool.check_repository, - '/path/to/test.git', - username='myuser', - password='mypass', - local_site_name=None) - finally: - GitTool.auth_form = old_auth_form - GitTool.repository_form = old_repository_form - GitTool.check_repository = old_check_repository - @unittest.skipIf(P4 is None, 'P4 module is not installed') def test_plain_repository_with_prefers_mirror_path(self): """Testing RepositoryForm with a plain repository and diff --git a/reviewboard/scmtools/tests/test_repository_manager.py b/reviewboard/scmtools/tests/test_repository_manager.py index f219523f1a8e98484be450865fafc2655330ad29..57fbb19387a6950e395963ce473e79904746994d 100644 --- a/reviewboard/scmtools/tests/test_repository_manager.py +++ b/reviewboard/scmtools/tests/test_repository_manager.py @@ -3,7 +3,6 @@ from django.contrib.auth.models import AnonymousUser from djblets.testing.decorators import add_fixtures -from reviewboard.deprecation import RemovedInReviewBoard60Warning from reviewboard.scmtools.models import Repository from reviewboard.site.models import LocalSite from reviewboard.testing import TestCase @@ -208,17 +207,6 @@ class RepositoryManagerTests(TestCase): Repository.objects.accessible(user, local_site=LocalSite.ALL)) - message = ( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - self.assertIn( - repository, - Repository.objects.accessible(user, show_all_local_sites=True)) - def test_accessible_ids_with_public(self): """Testing Repository.objects.accessible_ids with public repository""" user = self.create_user() @@ -415,18 +403,6 @@ class RepositoryManagerTests(TestCase): user, local_site=LocalSite.ALL)) - message = ( - 'show_all_local_sites is deprecated. Please pass ' - 'local_site=LocalSite.ALL instead. This will be required ' - 'in Review Board 6.' - ) - - with self.assertWarns(RemovedInReviewBoard60Warning, message): - self.assertIn( - repository.pk, - Repository.objects.accessible_ids(user, - show_all_local_sites=True)) - def test_get_best_match_with_pk(self): """Testing Repository.objects.get_best_match with repository ID""" repository1 = self.create_repository() diff --git a/reviewboard/webapi/resources/filediff.py b/reviewboard/webapi/resources/filediff.py index e93214d5ee3576f46deb3907c3cd9a07e2815cbf..34b78262b0a5ad5a9c7125826c28da1e57ec9181 100644 --- a/reviewboard/webapi/resources/filediff.py +++ b/reviewboard/webapi/resources/filediff.py @@ -561,7 +561,7 @@ class FileDiffResource(WebAPIResource): # XXX: Kind of a hack. api_format = mimetype.split('+')[-1] - resp = WebAPIResponse(request, payload, api_format=api_format) + resp = WebAPIResponse(request, obj=payload, api_format=api_format) set_last_modified(resp, filediff.diffset.timestamp) return resp