Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Use StrCollection in more places #14809

Open
clokep opened this issue Jan 10, 2023 · 4 comments
Open

Use StrCollection in more places #14809

clokep opened this issue Jan 10, 2023 · 4 comments
Labels
O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain

Comments

@clokep
Copy link
Member

clokep commented Jan 10, 2023

Using a Sequence[str] or Collection[str] is bad since a str also fits those and you usually want this to refer to a Set[str], List[str], Tuple[str, ...], etc.

In #14716 @reivilibre added a StrCollection type which we can re-use for this.

This has bit us a few times in the past but I'm failing to find references at the moment.

It would be good to audit Sequence[str] and Collection[str] and see if any of those should be updated.

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. O-Occasional Affects or can be seen by some users regularly or most users rarely labels Jan 10, 2023
@clokep
Copy link
Member Author

clokep commented Jan 26, 2023

I was using grep -rE "(Collection|Sequence)\[str\]" <path> to attempt to find these.

@clokep
Copy link
Member Author

clokep commented Jan 26, 2023

I'm assuming Collection[bytes] and Sequence[bytes] have the same issue? I couldn't think of any other potential examples though?

@clokep
Copy link
Member Author

clokep commented Feb 25, 2023

I'm assuming Collection[bytes] and Sequence[bytes] have the same issue? I couldn't think of any other potential examples though?

Or rather bytes would match Collection[int], etc. I'm not sure we need to "protect" against this as it seems like a less likely mistake to make.

@clokep
Copy link
Member Author

clokep commented Feb 25, 2023

I think we also want to look at Iterable[str] and Container[str] FTR, so the grep might now be

grep -rE "(Collection|Sequence|Iterable|Container)\[str\]" ....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain
Projects
None yet
Development

No branches or pull requests

2 participants