Summary

Update typing and other modernization in git client.

Review Request #14324 — Created Feb. 3, 2025 and updated 1 month, 2 weeks ago

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.	3eb174ba8faa7ed1a7736dd0c9256f728b22ab1d

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	2 months, 3 weeks ago
Instead of casting (here and below), we should probably assert.	chipx86	2 months, 3 weeks ago
Instead of a cast, let's assert instead.	chipx86	2 months, 3 weeks ago
For here and other messages, I think the right move is to keep these as-is and start wrapping in gettext. …	chipx86	2 months, 3 weeks ago
Small thing, but when just consuming, we should use Sequence.	chipx86	2 months, 3 weeks ago
Can you put these in alphabetical order?	chipx86	2 months, 3 weeks ago
Nit: I think this should be Union[str, Sequence[str]]	maubin	2 months, 1 week ago
Nit: I think this should be Sequence[bytes]	maubin	2 months, 1 week ago
Same here.	maubin	2 months, 1 week ago
Also a nit: While here can you wrap None in double backticks	maubin	2 months, 1 week ago
Same Sequence nit here.	maubin	2 months, 1 week ago
Same Sequence nit here.	maubin	2 months, 1 week ago
Since we're changing this, we should maybe use Sequence now.	chipx86	1 month, 2 weeks ago
We shouldn't override types in subclasses. They should be defined once in the parent, so the type checker can verify …	chipx86	1 month, 2 weeks ago
This should probably be a Mapping.	chipx86	1 month, 2 weeks ago
Localized strings with multiple variables need to use names instead of positional markers. Also, why the change in the code …	chipx86	1 month, 2 weeks ago
This should maybe return a Sequence, unless we should be modifying the result.	chipx86	1 month, 2 weeks ago
This needs to continue using named placeholders.	chipx86	1 month, 2 weeks ago
Here, too.	chipx86	1 month, 2 weeks ago
This should maybe be a Mapping.	chipx86	1 month, 2 weeks ago
This type is set in the parent, so we shouldn't override it here.	chipx86	1 month, 2 weeks ago
multiple spaces before operator Column: 17 Error code: E221	reviewbot	1 month, 2 weeks ago
multiple spaces before operator Column: 22 Error code: E221	reviewbot	1 month, 2 weeks ago
multiple spaces before operator Column: 20 Error code: E221	reviewbot	1 month, 2 weeks ago
multiple spaces before operator Column: 35 Error code: E221	reviewbot	1 month, 2 weeks ago
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	1 month, 2 weeks ago
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	1 month, 2 weeks ago
There are no open issues

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 1 month, 2 weeks ago

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.