Summary

Add types and assertions for parts of the Perforce diff logic.

Review Request #12618 — Created Sept. 22, 2022 and submitted Sept. 28, 2022, 12:52 a.m.

Information

Owner

chipx86

Repository

RBTools

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Perforce has some complex diff logic, and not all of it is instrumented
or even comprehensively documented based on actual usage.

In preparation for work on modernizing the diff building, this change
adds type annotations for all the _extract*() functions used to
calculate add/delete/edit/move information, and updates documentation
accordingly. To keep code readable, these functions now require keyword
arguments, and flags have defaults.

As part of this, mypy and pyright determined there were places where
None values could theoretically be passed in as paths or revisions in
some places. This would have led to broken diffs before, but shouldn't
really happen. We now assert these values as a precaution.

It was also found that revisions are sometimes passed as a string, and
sometimes as an integer. The functions themselves can handle either, but
we now formalize this in the type annotations. We may wish to
standardize it at a later date.

Testing Done

Unit tests pass.

Commits

Summary	ID
Add types and assertions for parts of the Perforce diff logic. Perforce has some complex diff logic, and not all of it is instrumented or even comprehensively documented based on actual usage. In preparation for work on modernizing the diff building, this change adds type annotations for all the `_extract*()` functions used to calculate add/delete/edit/move information, and updates documentation accordingly. To keep code readable, these functions now require keyword arguments, and flags have defaults. As part of this, mypy and pyright determined there were places where `None` values could theoretically be passed in as paths or revisions in some places. This would have led to broken diffs before, but shouldn't really happen. We now assert these values as a precaution. It was also found that revisions are sometimes passed as a string, and sometimes as an integer. The functions themselves can handle either, but we now formalize this in the type annotations. We may wish to standardize it at a later date.	fcdec8230adaa7baae620226c23c3d6f12da9752

Summary

Add types and assertions for parts of the Perforce diff logic.

Perforce has some complex diff logic, and not all of it is instrumented or even comprehensively documented based on actual usage. In preparation for work on modernizing the diff building, this change adds type annotations for all the `_extract*()` functions used to calculate add/delete/edit/move information, and updates documentation accordingly. To keep code readable, these functions now require keyword arguments, and flags have defaults. As part of this, mypy and pyright determined there were places where `None` values could theoretically be passed in as paths or revisions in some places. This would have led to broken diffs before, but shouldn't really happen. We now assert these values as a precaution. It was also found that revisions are sometimes passed as a string, and sometimes as an integer. The functions themselves can handle either, but we now formalize this in the type annotations. We may wish to standardize it at a later date.

fcdec8230adaa7baae620226c23c3d6f12da9752

Issues

Description	From	Last Updated
We could move this assert outside of the try block. Just makes it more consistent with the other places where …	maubin	Sept. 22, 2022, 12:56 p.m.

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

We could move this assert outside of the try block. Just makes it more consistent with the other places where you added assertions.

Change Summary:

Moved an assert out of a try.

Commits:

	Summary	ID
	Add types and assertions for parts of the Perforce diff logic. Perforce has some complex diff logic, and not all of it is instrumented or even comprehensively documented based on actual usage. In preparation for work on modernizing the diff building, this change adds type annotations for all the `_extract*()` functions used to calculate add/delete/edit/move information, and updates documentation accordingly. To keep code readable, these functions now require keyword arguments, and flags have defaults. As part of this, mypy and pyright determined there were places where `None` values could theoretically be passed in as paths or revisions in some places. This would have led to broken diffs before, but shouldn't really happen. We now assert these values as a precaution. It was also found that revisions are sometimes passed as a string, and sometimes as an integer. The functions themselves can handle either, but we now formalize this in the type annotations. We may wish to standardize it at a later date.	807357716d4f4a5b6a1bba4e6e48dd0a17b72dca
	Add types and assertions for parts of the Perforce diff logic. Perforce has some complex diff logic, and not all of it is instrumented or even comprehensively documented based on actual usage. In preparation for work on modernizing the diff building, this change adds type annotations for all the `_extract*()` functions used to calculate add/delete/edit/move information, and updates documentation accordingly. To keep code readable, these functions now require keyword arguments, and flags have defaults. As part of this, mypy and pyright determined there were places where `None` values could theoretically be passed in as paths or revisions in some places. This would have led to broken diffs before, but shouldn't really happen. We now assert these values as a precaution. It was also found that revisions are sometimes passed as a string, and sometimes as an integer. The functions themselves can handle either, but we now formalize this in the type annotations. We may wish to standardize it at a later date.	fcdec8230adaa7baae620226c23c3d6f12da9752

Diff:

Revision 2 (+274 -92)

Show changes

rbtools/clients/perforce.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (f869776)