Add enhanced support for review request matching.

Review Request #12263 — Created April 26, 2022 and submitted

Information

RBTools
release-3.x

Reviewers

Our review request matching ("guessing") code has been around for a very
long time. It worked by comparing the summary and description of pending
review requests to the local tree, computing a score, and returning
either an exact match (100% equal) or a fuzzy match. Fuzzy matches would
be presented to the user so that they could determine which is the
correct review request to update.

This change greatly enhances this logic, deprecating a lot of old code
in the process. Review requests can now be matched based on the commit
ID, or based on review request extra_data content (if the SCMClient
implementation supports it).

Matching is done in three stages now:

  1. Checks for an exact commit ID match. This is considered an exact
    match (mostly useful when matching without changing something that
    impacts the commit ID -- this could be used in time to ease attaching
    files or publishing changes, for example).

  2. Checks each review request using SCMClient-provided logic, using
    the new SCMClient.get_tree_matches_review_request() method, which
    can check extra_data for any state it wishes to check. This can
    return True if there's an exact match, False if a review request
    should be outright rejected (skipping any future matching logic), or
    None to proceed to fuzzier matching.

  3. Checks the summary and description against the tree, as before. This
    can generate an exact match or a list of fuzzy matches.

If there's only one exact match, then it wins.

If there's more than one exact match, they're considered high-priority
fuzzy matches, appended to any other fuzzy matches found.

This will give us a lot of options down the road for enhancing
operations like posting/updating review requests, closing review
requests, etc.

Short-term, SOS is the only tool that utilizes the extra_data
matching, and will match a review request if the workarea ID and
changeset ID stored when creating/updating a review request both match
the local workarea.

There are a few deprecations and related changes:

  • Much of the older match code (the Score object,
    get_possible_matches(), num_exact_matches()) has been deprecated
    and will be removed in RBTools 4.0.

  • The guess_summary and guess_description arguments to
    guess_existing_review_request are deprecated as well, as they were
    required for older logic anyway, and all internal uses hard-code these
    to True.

  • submit_as will be required in RBTools 4.0. This function will
    require an explicit username and an active session going forward,
    to reduce the number of places that can trigger a login flow.

  • An exception for when review_requests was falsy has been removed,
    as it was checking if a ListResource was falsy. This never got hit
    because resources are (currently) never falsy.

Some comments have been added with planned deprecations to make in
RBTools 4.0.

Unit tests were added for non-deprecated functionality.

Unit tests all pass.

Tested this with updating a variety of Git-based changes (which do not
use SCMClient matching) and SOS changes (which do). I haven't encountered
any regressions or noticeable changes in behavior.

Summary ID
Add enhanced support for review request matching.
Our review request matching ("guessing") code has been around for a very long time. It worked by comparing the summary and description of pending review requests to the local tree, computing a score, and returning either an exact match (100% equal) or a fuzzy match. Fuzzy matches would be presented to the user so that they could determine which is the correct review request to update. This change greatly enhances this logic, deprecating a lot of old code in the process. Review requests can now be matched based on the commit ID, or based on review request `extra_data` content (if the `SCMClient` implementation supports it). Matching is done in three stages now: 1. Checks for an exact commit ID match. This is considered an exact match (mostly useful when matching without changing something that impacts the commit ID -- this could be used in time to ease attaching files or publishing changes, for example). 2. Checks each review request using `SCMClient`-provided logic, using the new `SCMClient.get_tree_matches_review_request()` method, which can check `extra_data` for any state it wishes to check. This can return `True` if there's an exact match, `False` if a review request should be outright rejected (skipping any future matching logic), or `None` to proceed to fuzzier matching. 3. Checks the summary and description against the tree, as before. This can generate an exact match or a list of fuzzy matches. If there's only one exact match, then it wins. If there's more than one exact match, they're considered high-priority fuzzy matches, appended to any other fuzzy matches found. This will give us a lot of options down the road for enhancing operations like posting/updating review requests, closing review requests, etc. Short-term, SOS is the only tool that utilizes the `extra_data` matching, and will match a review request if the workarea ID and changeset ID stored when creating/updating a review request both match the local workarea. There are a few deprecations and related changes: * Much of the older match code (the `Score` object, `get_possible_matches(), `num_exact_matches()`) has been deprecated and will be removed in RBTools 4.0. * The `guess_summary` and `guess_description` arguments to `guess_existing_review_request` are deprecated as well, as they were required for older logic anyway, and all internal uses hard-code these to `True`. * `submit_as` will be required in RBTools 4.0. This function will require an explicit username and an active session going forward, to reduce the number of places that can trigger a login flow. * An exception for when `review_requests` was falsy has been removed, as it was checking if a `ListResource` was falsy. This never got hit because resources are (currently) never falsy. Some comments have been added with planned deprecations to make in RBTools 4.0. Unit tests were added for non-deprecated functionality.
b02ce83aaaff4af4d269e75f2ac22b996240692c
Description From Last Updated

F401 'json' imported but unused

reviewbotreviewbot

F401 'six.moves.urllib.request.urlopen' imported but unused

reviewbotreviewbot

F401 'rbtools.testing.URLMapTransport' imported but unused

reviewbotreviewbot

F841 local variable 'transport' is assigned to but never used

reviewbotreviewbot

F841 local variable 'pending_review_requests' is assigned to but never used

reviewbotreviewbot

F841 local variable 'transport' is assigned to but never used

reviewbotreviewbot

F841 local variable 'pending_review_requests' is assigned to but never used

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

flake8

chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (92fe2c9)