Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce clearer usage of IsList() etc. with no arguments #96

Open
alexmojaki opened this issue Jan 18, 2024 · 3 comments · May be fixed by #106
Open

Enforce clearer usage of IsList() etc. with no arguments #96

alexmojaki opened this issue Jan 18, 2024 · 3 comments · May be fixed by #106

Comments

@alexmojaki
Copy link
Contributor

IsList() with no arguments is equivalent to an empty list, but the user probably just wants to check for a list with any number of items. It should throw an error suggesting that the user write IsList(length=0) or IsList(length=...) to prevent ambiguity. Similar for IsTuple(). Maybe even IsListOrTuple().

@samuelcolvin
Copy link
Owner

Yes true, and IsDict, PR welcome.

@FBruzzesi
Copy link
Contributor

Hey there! I am taking a look at this and coming up short due to the following: by raising an error during the initialization, then

[] == IsListOrTuple

would also raise as is falls back to ln24-25 in DirtyEqualsMeta:

try:
    return self() == other
...

While I agree that IsListOrTuple() is ambiguous, I would argue that something == IsListOrTuple should still be allowed.

Any tips?

@alexmojaki
Copy link
Contributor Author

@samuelcolvin will need to confirm, but in my opinion it's fine to just not allow something == IsListOrTuple. That's also still ambiguous, exactly as much as something == IsListOrTuple(). IsListOrTuple is a bit murkier than IsList because at least it allows two possibilities. something == IsList is pointless to allow with its current behaviour because it could just be written as something == [] so that's probably not what the user intends when they write it.

@FBruzzesi FBruzzesi linked a pull request Sep 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants