Summary

ClearCase updates part 2: typing and consolidation.

Review Request #13606 — Created March 4, 2024 and submitted April 1, 2024, 3:17 p.m.

Information

Owner

david

Repository

RBTools

Branch

release-5.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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.

Commits

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

Summary

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

Issues

Description	From	Last Updated
TYPE_CHECKING sorts before Tuple.	chipx86	March 4, 2024, 4:54 p.m.
TypeAlias sorts before TypedDict.	chipx86	March 4, 2024, 4:54 p.m.
These can probably live in TYPE_CHECKING.	chipx86	March 4, 2024, 4:54 p.m.
If we're renaming the class and thus breaking API anyway, can we make these keyword-only?	chipx86	March 4, 2024, 4:55 p.m.
This should probably be typed.	chipx86	March 4, 2024, 4:55 p.m.
This should probably be typed.	chipx86	March 4, 2024, 4:56 p.m.
This too (OrderedDict[type, type], I think?)	chipx86	March 4, 2024, 4:56 p.m.
Is the terminology in ClearCase a "change set" or a "changeset"? Also, hmm... not sure what the right option is …	chipx86	March 4, 2024, 6:32 p.m.
You can feel free to ignore this, but I wonder if it'd be better to have some constant that we …	chipx86	March 4, 2024, 6:29 p.m.
Doesn't have to be this change, but filing an issue for tracking: We should update to use run_process instead of …	chipx86	March 4, 2024, 4:58 p.m.
I don't care enough about this in this file, but the ) on its own line is nicer for multi-line …	chipx86	March 4, 2024, 6:29 p.m.
Can we make these keyword-only?	chipx86	March 26, 2024, 3:56 p.m.
Blank line between these.	chipx86	March 26, 2024, 3:56 p.m.
These might all be good candidates for a @cached_property, I think?	chipx86	March 26, 2024, 4 p.m.
Just to check, are there any chances of a false-positive with this check? Can what looks like a tag appear …	chipx86	March 26, 2024, 3:59 p.m.

flake8 passed.

JSHint passed.

rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
TYPE_CHECKING sorts before Tuple.
```
rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
TypeAlias sorts before TypedDict.
```
rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
These can probably live in TYPE_CHECKING.
```

rbtools/clients/clearcase.py (Diff revision 1)

The issue has been resolved. Show all issues

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

rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably be typed.
```
rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably be typed.
```
rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This too (OrderedDict[type, type], I think?)
```

rbtools/clients/clearcase.py (Diff revision 1)

The issue has been resolved. 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?

david March 4, 2024, 6:32 p.m.

For activities, their documentation says "change set".

rbtools/clients/clearcase.py (Diff revision 1)

The issue has been resolved. 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.

rbtools/clients/clearcase.py (Diff revision 1)

The issue has been dropped. 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.

david March 4, 2024, 6:32 p.m.

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

rbtools/clients/tests/test_clearcase.py (Diff revision 1)

The issue has been resolved. 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 { ... }.

Commits:

	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.	bffbfe8e57ab3ef2c7d6e0c00cf7d7b2377951a3
	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.	a5d676da0ca3be02dfd651b743274feb41e7daa9

Diff:

Revision 2 (+1120 -830)

Show changes

	rbtools/clients/clearcase.py
	rbtools/clients/tests/test_clearcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbtools/clients/clearcase.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we make these keyword-only?
```
rbtools/clients/clearcase.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

rbtools/clients/clearcase.py (Diff revision 2)

The issue has been dropped. Show all issues

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

david March 26, 2024, 4:01 p.m.

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.

chipx86 April 1, 2024, 2:55 p.m.

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

rbtools/clients/clearcase.py (Diff revision 2)

The issue has been dropped. 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?

david March 26, 2024, 4:01 p.m.

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.

Commits:

	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.	a5d676da0ca3be02dfd651b743274feb41e7daa9
	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

Diff:

Revision 3 (+1140 -846)

Show changes

	rbtools/clients/clearcase.py
	rbtools/clients/tests/test_clearcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.x (d816392)