Summary

Update typing and other modernization in git client.

Review Request #14324 — Created Feb. 3, 2025 and submitted June 10, 2025, 9:21 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This change does some improvements to our type hints, fixes up some doc
typos/errors, and moves to more modern syntax for a few things
(f-strings, list construction, etc).

Testing Done

Ran unit tests.

Commits

Summary	ID
Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	e3a0b6882dd5dbae4a4e3531a0d3d855b6bae9e1

Issues

Description	From	Last Updated
At this point it'd be nice to just wrap these as a typical multi-line list with one item per line. …	chipx86	Feb. 10, 2025, 4:17 p.m.
Instead of casting (here and below), we should probably assert.	chipx86	Feb. 10, 2025, 4:18 p.m.
Instead of a cast, let's assert instead.	chipx86	Feb. 10, 2025, 4:19 p.m.
For here and other messages, I think the right move is to keep these as-is and start wrapping in gettext. …	chipx86	Feb. 10, 2025, 4:32 p.m.
Small thing, but when just consuming, we should use Sequence.	chipx86	Feb. 10, 2025, 4:22 p.m.
Can you put these in alphabetical order?	chipx86	Feb. 10, 2025, 4:23 p.m.
Nit: I think this should be Union[str, Sequence[str]]	maubin	Feb. 24, 2025, 6:02 p.m.
Nit: I think this should be Sequence[bytes]	maubin	Feb. 24, 2025, 6:02 p.m.
Same here.	maubin	Feb. 24, 2025, 6:02 p.m.
Also a nit: While here can you wrap None in double backticks	maubin	Feb. 24, 2025, 6:02 p.m.
Same Sequence nit here.	maubin	Feb. 24, 2025, 6:04 p.m.
Same Sequence nit here.	maubin	Feb. 24, 2025, 6:04 p.m.
Since we're changing this, we should maybe use Sequence now.	chipx86	March 17, 2025, 2:49 p.m.
We shouldn't override types in subclasses. They should be defined once in the parent, so the type checker can verify …	chipx86	March 17, 2025, 2:53 p.m.
This should probably be a Mapping.	chipx86	March 17, 2025, 2:53 p.m.
Localized strings with multiple variables need to use names instead of positional markers. Also, why the change in the code …	chipx86	March 17, 2025, 3:10 p.m.
This should maybe return a Sequence, unless we should be modifying the result.	chipx86	March 17, 2025, 3:05 p.m.
This needs to continue using named placeholders.	chipx86	March 17, 2025, 3:11 p.m.
Here, too.	chipx86	March 17, 2025, 3:11 p.m.
This should maybe be a Mapping.	chipx86	March 17, 2025, 3:06 p.m.
This type is set in the parent, so we shouldn't override it here.	chipx86	March 17, 2025, 3:06 p.m.
multiple spaces before operator Column: 17 Error code: E221	reviewbot	March 17, 2025, 3:18 p.m.
multiple spaces before operator Column: 22 Error code: E221	reviewbot	March 17, 2025, 3:18 p.m.
multiple spaces before operator Column: 20 Error code: E221	reviewbot	March 17, 2025, 3:18 p.m.
multiple spaces before operator Column: 35 Error code: E221	reviewbot	March 17, 2025, 3:18 p.m.
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	March 17, 2025, 3:18 p.m.
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	March 17, 2025, 3:19 p.m.

flake8 passed.

JSHint passed.

Very nice cleanup. Got a few small suggestions, mostly around ensuring type safety.

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

The issue has been resolved. Show all issues

At this point it'd be nice to just wrap these as a typical multi-line list with one item per line.
We should probably also type as ClassVar[Sequence[str]]

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Instead of casting (here and below), we should probably assert.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Instead of a cast, let's assert instead.
```

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

The issue has been resolved. Show all issues

For here and other messages, I think the right move is to keep these as-is and start wrapping in gettext. The newer bits of code (Patcher, Patch, soem errors, a couple SCMs) are using gettext.

rbtools/clients/tests/test_git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Small thing, but when just consuming, we should use Sequence.
```

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

The issue has been resolved. Show all issues

Can you put these in alphabetical order?

Commits:

	Summary	ID
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	5a94a36e4846bf02894202f95ffb8f1d7606d564
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	5457bdd03805fe4fca704737365ddbad65c0b24b

Diff:

Revision 2 (+938 -562)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests/test_git.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Nit: I think this should be Union[str, Sequence[str]]
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Nit: I think this should be Sequence[bytes]
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Also a nit: While here can you wrap None in double backticks
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same Sequence nit here.
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same Sequence nit here.
```

Change Summary:

Make requested fixes (plus another couple Sequence changes that I found)

Commits:

	Summary	ID
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	5457bdd03805fe4fca704737365ddbad65c0b24b
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	ec5312625ffb8a58d94f15084bb6ce12dff411fd

Diff:

Revision 3 (+950 -568)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests/test_git.py
	rbtools/utils/diffs.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Since we're changing this, we should maybe use Sequence now.
```

rbtools/clients/git.py (Diff revision 3)

The issue has been resolved. Show all issues

We shouldn't override types in subclasses. They should be defined once in the parent, so the type checker can verify the assigned value against the parent type. Same goes for the ones below.

rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This should probably be a Mapping.
```

rbtools/clients/git.py (Diff revision 3)

The issue has been resolved. Show all issues

Localized strings with multiple variables need to use names instead of positional markers.

Also, why the change in the code to .format() instead of %? I'm not opposed, but we've never really bothered with the former before.

david March 17, 2025, 3:14 p.m.

I feel like we should be moving towards f-strings and .format() as a rule, since it's generally more readable and has a lot fewer quirks.

rbtools/clients/git.py (Diff revision 3)

The issue has been resolved. Show all issues

This should maybe return a Sequence, unless we should be modifying the result.

rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This needs to continue using named placeholders.
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Here, too.
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This should maybe be a Mapping.
```
rbtools/clients/tests/test_git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This type is set in the parent, so we shouldn't override it here.
```

Commits:

	Summary	ID
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	ec5312625ffb8a58d94f15084bb6ce12dff411fd
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	ff9479e44b3dc471d00792fda5ba9627844fce58

Diff:

Revision 4 (+952 -550)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests/test_git.py
	rbtools/utils/diffs.py
	rbtools/utils/process.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/clients/git.py (Diff revision 4)
The issue has been dropped. Show all issues
```
multiple spaces before operator

Column: 17
Error code: E221
```
rbtools/clients/git.py (Diff revision 4)
The issue has been dropped. Show all issues
```
multiple spaces before operator

Column: 22
Error code: E221
```
rbtools/clients/git.py (Diff revision 4)
The issue has been dropped. Show all issues
```
multiple spaces before operator

Column: 20
Error code: E221
```
rbtools/clients/git.py (Diff revision 4)
The issue has been dropped. Show all issues
```
multiple spaces before operator

Column: 35
Error code: E221
```
rbtools/utils/process.py (Diff revision 4)
The issue has been dropped. Show all issues
```
line too long (81 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	ff9479e44b3dc471d00792fda5ba9627844fce58
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	e7a8b082115d25662179d1c709dca5770d51ff5b

Diff:

Revision 5 (+950 -548)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests/test_git.py
	rbtools/utils/diffs.py
	rbtools/utils/process.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/utils/process.py (Diff revision 5)
The issue has been dropped. Show all issues
```
line too long (81 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	e7a8b082115d25662179d1c709dca5770d51ff5b
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	3eb174ba8faa7ed1a7736dd0c9256f728b22ab1d

Diff:

Revision 6 (+952 -548)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests/test_git.py
	rbtools/utils/diffs.py
	rbtools/utils/process.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Switch to new union syntax, use a named logger, and move additional stuff into TYPE_CHECKING.

Commits:

	Summary	ID
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	3eb174ba8faa7ed1a7736dd0c9256f728b22ab1d
	Update typing and other modernization in git client. This change does some improvements to our type hints, fixes up some doc typos/errors, and moves to more modern syntax for a few things (f-strings, list construction, etc). Testing Done: Ran unit tests.	e3a0b6882dd5dbae4a4e3531a0d3d855b6bae9e1

Diff:

Revision 7 (+1050 -680)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests/test_git.py
	rbtools/utils/diffs.py
	rbtools/utils/process.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (9e4b1d4)