Summary

Don't use mutable defaults for SCMClient methods.

Review Request #14471 — Created June 24, 2025 and updated July 17, 2025, 10:06 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This change updates the parse_revision_spec, diff, and

create_commit methods for SCMClients to avoid using mutable lists as

defaults for arguments. These are now set to use

(Sequence[str] | None) = None, and initialize the values in the body

of the method instead.
Because all "real" call sites pass these in (as empty lists), I've

added deprecation warnings that we will be removing the default value

(and the | None) in rbtools 8.

Testing Done

Ran unit tests.

Commits

Summary	ID
Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	f4d05933ad0520da8f5ce34d9a9b67a699905a3d

Summary

Don't use mutable defaults for SCMClient methods.

This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] | None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `| None`) in rbtools 8. Testing Done: Ran unit tests.

f4d05933ad0520da8f5ce34d9a9b67a699905a3d

Issues

Description	From	Last Updated
'typing.List' imported but unused Column: 1 Error code: F401	reviewbot	June 24, 2025, 3 p.m.
'typing.List' imported but unused Column: 1 Error code: F401	reviewbot	June 24, 2025, 3 p.m.
Why do we need to add \| None here?	maubin	July 10, 2025, 3:03 p.m.
If calling the SCMClient's diff() directly, I think it's reasonable to be able to leave off these argument. I think …	chipx86	July 17, 2025, 9:59 p.m.
Should this assert, or should we fall back to a value? This might be called manually be a caller, not …	chipx86	July 17, 2025, 9:58 p.m.
Isn't the is not None check redundant since we check if its a dict right after?	maubin	July 10, 2025, 3:03 p.m.
'typing.Type' imported but unused Column: 1 Error code: F401	reviewbot	July 17, 2025, 10:05 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/cvs.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'typing.List' imported but unused

Column: 1
Error code: F401
```
rbtools/clients/plastic.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'typing.List' imported but unused

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	25d4f739ac3024bbea14f0a15d970c21c3144268
	Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	c90a2de378ec581efb1f70e7e470abb1844ef5d5

Diff:

Revision 2 (+1274 -536)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/jujutsu.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been dropped. Show all issues

Why do we need to add | None here?

david July 10, 2025, 3:03 p.m.

It's just to bring it into sync with the signature of the method in the base class.

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

It would be nice to have a "This argument will be mandatory" thing in housekeeping. Not necessary for this change though.

rbtools/clients/tests/__init__.py (Diff revision 2)

The issue has been dropped. Show all issues

Isn't the is not None check redundant since we check if its a dict right after?

david July 10, 2025, 3:03 p.m.

Kinda sorta. type checkers don't seem to treat assertIsInstance the same as assert isinstance() when it comes to type narrowing.

chipx86 July 14, 2025, 3:12 p.m.

Yeah, no built-in support for that. Python doesn't have the right primitives for assertIsInstance (TypeIs and TypeGuard both impose some constraints on how the function operates that isn't right for assertIsInstance). Pretty annoying that this case isn't covered.

Ship it!

```
Ship It!
```

This is overall good, but two things stand out:

diff() is being treated as an internal function, with optional arguments now made mandatory. I don't think we should be doing this. This method can be called by utility scripts, and as those arguments aren't required for building a standard diff, I believe they should remain optional. (This would also mean undoing the unit test changes that pass in empty values so that we can make sure they remain optional).
This makes changes to Plastic that are going to conflict with pending work on my end. Can we ignore this module for now?

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

The issue has been resolved. Show all issues

If calling the SCMClient's diff() directly, I think it's reasonable to be able to leave off these argument. I think they should stay optional.

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

The issue has been resolved. Show all issues

Should this assert, or should we fall back to a value? This might be called manually be a caller, not just internally.

Commits:

	Summary	ID
	Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	c90a2de378ec581efb1f70e7e470abb1844ef5d5
	Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	d18d727f97c1c748d7233ed93d06cbf78e89101c

Diff:

Revision 3 (+688 -274)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/jujutsu.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/clients/tests/__init__.py (Diff revision 3)
The issue has been resolved. Show all issues
```
'typing.Type' imported but unused

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	d18d727f97c1c748d7233ed93d06cbf78e89101c
	Don't use mutable defaults for SCMClient methods. This change updates the `parse_revision_spec`, `diff`, and `create_commit` methods for SCMClients to avoid using mutable lists as defaults for arguments. These are now set to use `(Sequence[str] \| None) = None`, and initialize the values in the body of the method instead. Because all "real" call sites pass these in (as empty lists), I've added deprecation warnings that we will be removing the default value (and the `\| None`) in rbtools 8. Testing Done: Ran unit tests.	f4d05933ad0520da8f5ce34d9a9b67a699905a3d

Diff:

Revision 4 (+686 -274)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/jujutsu.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.