• 
      

    Add a central representation of settings for diff display.

    Review Request #12786 — Created Jan. 13, 2023 and submitted

    Information

    Review Board
    release-5.0.x

    Reviewers

    We have an increasing number of options that influence the display of
    diffs. Most of these are site-wide, but some are influenced by user
    settings (syntax highlighting in particular). Historically, while most
    of these were queried during diff render, their values weren't factored
    into any caching or ETags, requiring cache to be cleared for most to
    take effect.

    As new settings are coming for diff display, to govern code safety
    checkers, it was time to rethink the approach here.

    This change introduces reviewboard.diffviewer.settings.DiffSettings.
    This is an option that represents all the settings that go into the
    display of diffs generally. This includes things like whether syntax
    highlighting is enabled, or how many diffs are on a page. It does not
    include per-diff mode-specific settings, like whether to expand/collapse
    a given section.

    The DiffSettings object can be computed once and then passed around to
    the various parts of the diff machinery. This keeps things consistent,
    and avoids having to pass around individual bits of state or to
    re-compute them.

    It also contains a state_hash property, which returns a SHA256 of the
    DiffSettings state. That's now included in all cache keys and ETags.
    If any of these settings change, new cache keys and ETags are used. If
    they're changed back, the original cache keys and ETags are used. This
    avoids stale diffs and stays performant.

    The old syntax highlighting flags for DiffChunkGenerator,
    DiffRenderer, and populate_diff_chunks() have been deprecated, but
    are still supported until Review Board 6. diff_settings is now
    recommended, but each object will compute based on site defaults if not
    provided.

    This change intentionally does not add type hints for the functions that
    were modified to take diff_settings, as adding type hints for these
    functions is a surprisingly-large undertaking. I'll be revisiting those
    functions in the future.

    Unit tests pass on all supported versions of Python.

    Manually tested this with the various diff viewer settings, including
    some in-progress ones for code safety. Any time I changed a setting,
    reloading would give me new diffs, verifying that both cache keys and
    ETags had changed. Subsequent reloads with those settings used cached
    state.

    Summary ID
    Add a central representation of settings for diff display.
    We have an increasing number of options that influence the display of diffs. Most of these are site-wide, but some are influenced by user settings (syntax highlighting in particular). Historically, while most of these were queried during diff render, their values weren't factored into any caching or ETags, requiring cache to be cleared for most to take effect. As new settings are coming for diff display, to govern code safety checkers, it was time to rethink the approach here. This change introduces `reviewboard.diffviewer.settings.DiffSettings`. This is an option that represents all the settings that go into the display of diffs generally. This includes things like whether syntax highlighting is enabled, or how many diffs are on a page. It does not include per-diff mode-specific settings, like whether to expand/collapse a given section. The `DiffSettings` object can be computed once and then passed around to the various parts of the diff machinery. This keeps things consistent, and avoids having to pass around individual bits of state or to re-compute them. It also contains a `state_hash` property, which returns a SHA256 of the `DiffSettings` state. That's now included in all cache keys and ETags. If any of these settings change, new cache keys and ETags are used. If they're changed back, the original cache keys and ETags are used. This avoids stale diffs and stays performant. The old syntax highlighting flags for `DiffChunkGenerator`, `DiffRenderer`, and `populate_diff_chunks()` have been deprecated, but are still supported until Review Board 6. `diff_settings` is now recommended, but each object will compute based on site defaults if not provided.
    4c4e288856d51742cd7f06c43d6605dd232935e8
    Description From Last Updated

    undefined name 'List' Column: 14 Error code: F821

    reviewbotreviewbot

    Typo here and below "renderer" -> "rendered".

    maubinmaubin
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    chipx86
    maubin
    1. 
        
    2. reviewboard/diffviewer/diffutils.py (Diff revision 2)
       
       
      Show all issues

      Typo here and below "renderer" -> "rendered".

      1. Fixed all occurrences. Thanks!

    3. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-5.0.x (2041385)