Correct type of dependency_groups

[zedzhen]

close #39

---

📚 Documentation preview 📚: https://dependency-groups--40.org.readthedocs.build/en/40/

[sirosen]

Hi, thanks for catching this and filing #39!
We fixed this when we ported dependency-groups into packaging, but didn't backport the fix to here.

We should not use list, however, for public interfaces where a broader category of iterables are acceptable. Especially because list is invariant, so this change would make it a type-checking error to pass a list[str] to these interfaces (which is wrong).

Some of these could use Iterable, but in packaging we used Sequence, so I think we should do the same here.

[zedzhen]

https://github.com/zedzhen/dependency-groups/blob/868745d007537a5d70e817db1661349c6499f64a/src/dependency_groups/_implementation.py#L142-L144

        raw_group = self.dependency_groups[group]
        if not isinstance(raw_group, list):
            raise TypeError(f"Dependency group '{group}' is not a list")

raw_group must be list
self.dependency_groups values must be list
https://github.com/zedzhen/dependency-groups/blob/868745d007537a5d70e817db1661349c6499f64a/src/dependency_groups/_implementation.py#L85-L87

        self.dependency_groups, self._normalized_to_original = _normalize_group_names(
            dependency_groups
        )

https://github.com/zedzhen/dependency-groups/blob/868745d007537a5d70e817db1661349c6499f64a/src/dependency_groups/_implementation.py#L14-L39

def _normalize_group_names(
    dependency_groups: Mapping[str, list[str | Mapping[str, str]]],
) -> tuple[Mapping[str, list[str | Mapping[str, str]]], Mapping[str, str]]:
    """
    Normalize group names and return both normalized groups and reverse mapping.

    Returns a tuple of (normalized_groups, normalized_to_original).
    """
    original_names: dict[str, list[str]] = {}
    normalized_groups = {}
    normalized_to_original: dict[str, str] = {}


    for group_name, value in dependency_groups.items():
        normed_group_name = _normalize_name(group_name)
        original_names.setdefault(normed_group_name, []).append(group_name)
        normalized_groups[normed_group_name] = value
        normalized_to_original[normed_group_name] = group_name


    errors = []
    for normed_name, names in original_names.items():
        if len(names) > 1:
            errors.append(f"{normed_name} ({', '.join(names)})")
    if errors:
        raise ValueError(f"Duplicate dependency group names: {', '.join(errors)}")


    return normalized_groups, normalized_to_original

values of normalized_group is values dependency_groups.
Therefore, when changing the type from list to Iterable/Sequence, it will be necessary to turn the original type into a list.

If you think it's necessary, I'll add this comment to this PR.
compare: zedzhen/dependency-groups@fix-types...fix-types-iterable

My English is not very good, so maybe I didn't explain it very well.

[sirosen]

My English is not very good, so maybe I didn't explain it very well.

No worries! I don't think I'm having trouble understanding what you're saying.
I'm not sure I understand why you're saying it though:

Therefore, when changing the type from list to Iterable/Sequence, it will be necessary to turn the original type into a list.

Why would it be necessary for us to convert inputs to list?

If you look at the implementation in packaging you'll see that we use Sequence throughout, even for the isinstance check.

We can't fully copy in the packaging implementation at present because it has other differences.

---

list[T] and Sequence[T] behave very differently; I have written a long post about this, but the summary version is:

def f(x: list[int | str]): ...
def g(x: Sequence[int | str]): ...

y: list[str] = []

f(y)  # type checkers mark this as an error
g(y)  # this is okay

Sequence or Iterable would be appropriate for these interfaces. list is not.
I think we should use Sequence because that's what we used in packaging.

[zedzhen]

In line 143, we check that we have a list, and the type of this value exactly matches what we received as input. Therefore, it is necessary to convert to a list, or weaken this check.

[sirosen]

Therefore, it is necessary to convert to a list, or weaken this check.

Yes. Would expanding the check to allow any Sequence be a problem? Why or why not?