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

Rethink max_exclusive, convert to max_inclusive? #23

Closed
samuelcolvin opened this issue Sep 27, 2022 · 5 comments · Fixed by #24
Closed

Rethink max_exclusive, convert to max_inclusive? #23

samuelcolvin opened this issue Sep 27, 2022 · 5 comments · Fixed by #24

Comments

@samuelcolvin
Copy link
Contributor

samuelcolvin commented Sep 27, 2022

If we have MaxLen, and (in pydantic at least) that can be set via a max_length argument.

I really think this should mean "maximum inclusive", not "maximum exclusive" as currently documented.

This matches (IMHO) much better people's assumption about what MaxLen(5) or max_length=5 or Len(0, 5) means:

"The airbnb allows maximum 5 guests", you would assume 5 guests were allowed, not just 4

If for the sake of correctness, that involves either:

  • treating slices differently
  • or, removing the recommendation on allowing slices

That's sad, but I think a price worth paying.

At the end of the day max_length=5 meaning any length up to 4, won't fly in pydantic.

@samuelcolvin
Copy link
Contributor Author

My personal preference would be to treat slices e.g. 2:10 as Len(min_inclusive=2, max_inclusive=10).

But I can already hear the rumbling of disagreement as I write that.

@adriangb
Copy link
Contributor

max_length=5 meaning any length up to 4, won't fly

Yeah that is confusing to say the least!

either:

  • treating slices differently
  • or, removing the recommendation on allowing slices

My personal preference would be to remove slices.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 27, 2022

Yeah, this is a compelling argument. Let's remove slices and make both inclusive, and consider changing the name - min/max_len would match the other names, or min/max_size would match Hypothesis.

@samuelcolvin
Copy link
Contributor Author

Agreed, I'd prefer min_length / max_length to match MinLen / MaxLen / Len, but easy on that.

@samuelcolvin
Copy link
Contributor Author

I'll submit a PR once #21 is merged.

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