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

[Hotfix][OptionRule] Fix option rule about all connectors #3592

Merged
merged 10 commits into from
Nov 29, 2022

Conversation

EricJoy2048
Copy link
Member

No description provided.

Comment on lines -165 to -170
/**
* Conditional options, These options are required if the {@link Option} == expectValue.
*/
public <T> Builder conditional(Option<T> option, T expectValue, Option<?>... requiredOptions) {
return conditional(Condition.of(option, expectValue), requiredOptions);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, The tool method of a single expected value need not be deleted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, The tool method of a single expected value need not be deleted.

Ok.

Comment on lines 168 to 174
public Builder exclusive(@NonNull RequiredOption.ExclusiveRequiredOptions exclusiveRequiredOptions) {
exclusiveRequiredOptions.getExclusiveOptions().forEach(option -> {
verifyDuplicate(option, "ExclusiveOption");
});
this.requiredOptions.add(exclusiveRequiredOptions);
return this;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, delete the method.

Copy link
Contributor

@TaoZex TaoZex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Hisoka-X Hisoka-X merged commit 226dc6a into apache:dev Nov 29, 2022
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.

5 participants