ClearCase updates part 2: typing and consolidation.

Review Request #13606 — Created March 4, 2024 and submitted

Information

RBTools
release-5.x

Reviewers

This change is a somewhat more invasive refactor of the ClearCase
backend in RBTools.

To start with, we had several different definitions of what a
"changeset" was throughout the code. Some places used it to mean a list
of 2-tuples containing extended path pairs. Some places used it to mean
a list of 3-tuples containing a path and two versions. And finally, our
diff implementation used it to mean a list of ChangesetEntry objects.

I've reworked this to define a "changelist" as the list of extended path
pairs (aliased as _ChangedEntry), which is what gets returned by all
of the different methods for the various types of revision specs. This
then gets processed and turned into a "changeset" which is the list of
_ChangesetEntry objects. A couple of the revision types use one
additional type, _BranchedChangedEntry, which is the 3-tuple form.

We also had a number of methods that were unnecessarily implemented as
their own functions. These have for the most part been folded back into
the one place they got called from.

Most of the code that deals with changelists has been updated to yield
items rather than build new lists. In some cases we were rebuilding
lists up to 7 times.

This change also adds type hints to the rest of the implementation.

Ran unit tests.

Summary ID
ClearCase updates part 2: typing and consolidation.
This change is a somewhat more invasive refactor of the ClearCase backend in RBTools. To start with, we had several different definitions of what a "changeset" was throughout the code. Some places used it to mean a list of 2-tuples containing extended path pairs. Some places used it to mean a list of 3-tuples containing a path and two versions. And finally, our diff implementation used it to mean a list of `ChangesetEntry` objects. I've reworked this to define a "changelist" as the list of extended path pairs (aliased as `_ChangedEntry`), which is what gets returned by all of the different methods for the various types of revision specs. This then gets processed and turned into a "changeset" which is the list of `_ChangesetEntry` objects. A couple of the revision types use one additional type, `_BranchedChangedEntry`, which is the 3-tuple form. We also had a number of methods that were unnecessarily implemented as their own functions. These have for the most part been folded back into the one place they got called from. Most of the code that deals with changelists has been updated to yield items rather than build new lists. In some cases we were rebuilding lists up to 7 times. This change also adds type hints to the rest of the implementation. Testing Done: - Ran unit tests.
6f7b1638a22cfa4a24518a1dc9efe860615207a0
Description From Last Updated

TYPE_CHECKING sorts before Tuple.

chipx86chipx86

TypeAlias sorts before TypedDict.

chipx86chipx86

These can probably live in TYPE_CHECKING.

chipx86chipx86

If we're renaming the class and thus breaking API anyway, can we make these keyword-only?

chipx86chipx86

This should probably be typed.

chipx86chipx86

This should probably be typed.

chipx86chipx86

This too (OrderedDict[type, type], I think?)

chipx86chipx86

Is the terminology in ClearCase a "change set" or a "changeset"? Also, hmm... not sure what the right option is …

chipx86chipx86

You can feel free to ignore this, but I wonder if it'd be better to have some constant that we …

chipx86chipx86

Doesn't have to be this change, but filing an issue for tracking: We should update to use run_process instead of …

chipx86chipx86

I don't care enough about this in this file, but the ) on its own line is nicer for multi-line …

chipx86chipx86

Can we make these keyword-only?

chipx86chipx86

Blank line between these.

chipx86chipx86

These might all be good candidates for a @cached_property, I think?

chipx86chipx86

Just to check, are there any chances of a false-positive with this check? Can what looks like a tag appear …

chipx86chipx86
chipx86
  1. 
      
  2. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    TYPE_CHECKING sorts before Tuple.

  3. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    TypeAlias sorts before TypedDict.

  4. rbtools/clients/clearcase.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These can probably live in TYPE_CHECKING.

  5. rbtools/clients/clearcase.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    If we're renaming the class and thus breaking API anyway, can we make these keyword-only?

  6. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    This should probably be typed.

  7. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    This should probably be typed.

  8. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    This too (OrderedDict[type, type], I think?)

  9. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    Is the terminology in ClearCase a "change set" or a "changeset"?

    Also, hmm... not sure what the right option is here, but we shouldn't be printing directly, as that'll interfere with JSON output and such. I don't think we pass in these streams anywhere the client can get to it. Can we log this instead?

    1. For activities, their documentation says "change set".

  10. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    You can feel free to ignore this, but I wonder if it'd be better to have some constant that we can refer to that is assigned to sys.maxsize, rather than using sys.maxsize itself? I had to go look through the code to see what we were using this for. Having a constant could be more self-documenting.

  11. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Show all issues

    Doesn't have to be this change, but filing an issue for tracking: We should update to use run_process instead of execute.

    1. Yeah, I've been keeping track of that for all the clients I'm touching.

  12. rbtools/clients/tests/test_clearcase.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    I don't care enough about this in this file, but the ) on its own line is nicer for multi-line text in the same way that a trailing comma is nicer for multi-line lists/dicts. Easier to add new content without modifying a prior line. And it better matches the formatting used for multi-line lists/dicts, in terms of the [ ... ] or { ... }.

  13. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    Can we make these keyword-only?

  3. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

  4. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
    Show all issues

    These might all be good candidates for a @cached_property, I think?

    1. Hmm, perhaps. I don't feel like I understand the nuances of Python's version of cached_properly yet, though. I'd rather keep what we have since we know that works.

    2. Ah, right, Python's. Yeah, it's.. not an ideal decorator. Happy to drop this one.

  5. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues

    Just to check, are there any chances of a false-positive with this check? Can what looks like a tag appear elsewhere in the path?

    1. TBH I don't know for sure but we've been using this since the beginning and neither I nor anyone at HCL has reported a problem here.

  6. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.x (d816392)