Skip to content

Rename AtLeastOneOf new Constraint to OneOf #36722

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

Closed
wants to merge 2 commits into from
Closed

Rename AtLeastOneOf new Constraint to OneOf #36722

wants to merge 2 commits into from

Conversation

Neirda24
Copy link
Contributor

@Neirda24 Neirda24 commented May 6, 2020

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets PR: #35744
License MIT
Doc PR TODO ?

@przemyslaw-bogusz Originally created AtLeastOne constraint. Which it seems was merged in 5.1 as AtLeastOneOf. (#35744).

I propose to rename it simply OneOf because it stops as soon as one of the constraints has no violations. So At least and One of seems a duplicate of information. Also calling it just OneOf is to have the same wording as OpenApi spec.

@fancyweb
Copy link
Contributor

fancyweb commented May 6, 2020

I think OneOf is less precise because it can be interpreted as "XOR".

@Neirda24
Copy link
Contributor Author

Neirda24 commented May 6, 2020

@fancyweb 👍 : I didn't see it that way. But AtLeastOne suggest that many can succeed. But by the look of the code it stops as soon as a constraint has no violations. For me XOR equivalent would be ExactlyOneOrNone

@stof
Copy link
Member

stof commented May 6, 2020

@Neirda24 there is nothing in the current implementation forbidding multiple ones to succeed. We stop the loop once we know the final result (the validation succeeds), because it is useless to run all constraint validators if we already know the answer. The fact that it stops is an internal implementation detail to optimize performance, but it does not change the result.

@nicolas-grekas nicolas-grekas added this to the 5.1 milestone May 6, 2020
@Neirda24
Copy link
Contributor Author

Neirda24 commented May 6, 2020

@stof : The fact that it stops once a constraint has no violations means we don't check for others. If we don't we can't know if more would have succeeded. Meaning that at this time the validator can only assure us that One and only one constraint did succeed. And regarding the fact that it is exactly what we want I thought that AtLeastOneOf was a bit redundant.

@stof
Copy link
Member

stof commented May 6, 2020

@Neirda24 even if we would be running them all, we would only report a violation if all of them failed, as that's the only case where the AtLeastOne constraint is violated.
An AtLeastOne constraint does not care whether 1, 2 or 99999 constraints were satisfied to decide that it is valid.

The current implementation does not assure that one and only one constraint succeeded. To assure that only one part requires failing when 2+ are succeeding.

@przemyslaw-bogusz
Copy link
Contributor

@Neirda24 I changed the initial name to AtLeastOneOf due to a suggestion in the PR. As stof pointed out, the fact that it stops after one succeeds is an optimization. If the name is confusing in any way, maybe you could submit a PR to update the constraint's description in the documentation?

@stof
Copy link
Member

stof commented May 7, 2020

@przemyslaw-bogusz we don't care if others succeed or not. At least one means exactly that at least one of them succeed. The only way this can fail is if all of them fail. So as soon as one of them did not fail, we already know the outcome.

@nicolas-grekas
Copy link
Member

I'm personally fine with OneOf as it means the same as AtLeastOneOf to me.
A XOR behavior would be extremely surprising so I couldn't expect this to mean that.

@javiereguiluz
Copy link
Member

If we forget about the technical things for a moment, compare these two phrases:

Take one of these things.

Take at least one of these things.

To me, "one of" and "at least one of" have completely different meaning.

Returning to technical discussions ... "one of" to me is equivalent to {1} in regular expressions and "at least one of" is equivalent to {1,} ... so they are not the same either. But maybe I'm missing something here 🤔

@Neirda24
Copy link
Contributor Author

Neirda24 commented May 8, 2020

This is exactly my point. To me, the current validation does a {1} because (I understand it is for performance reasons) it stops checking other constraints as soon as one did succeed. If we run them all and then depending on whether at least one succeeded then {1,}.

In conclusion for me as long as we keep these optimization we change the behavior from At Least One Of to only One Of.

I don't know how it works. If there is a vote or something. So please just let me know if I can help in any way. (documentation as @przemyslaw-bogusz suggested).

@xabbuh
Copy link
Member

xabbuh commented May 9, 2020

IMO OneOf would be confusing exactly because of the {1} argument. It may lead users to think that they need to define a set of constraints where never more than one can pass at the same time.

@nicolas-grekas
Copy link
Member

I'm closing because this is not going to be accepted, let's move on. Thanks for proposing!

@stof
Copy link
Member

stof commented May 9, 2020

@Neirda24 the currently implementation does not do {1}. To do {1}, you have to report violations when 2 constraints (or more) are passing. The case where we can short-circuit the execution after one of them passes is precisely {1,} (where we only care about distinguishing between 0 and 1+ passing constraints).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants