-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
#10812 don't allow selecting a disabled option #10814
#10812 don't allow selecting a disabled option #10814
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #10814 +/- ##
=======================================
Coverage 50.46% 50.46%
=======================================
Files 84 84
Lines 5477 5477
Branches 278 278
=======================================
Hits 2764 2764
Misses 2435 2435
Partials 278 278 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! @asolntsev The changes look good to me. Other language bindings will also need to add the same checks.
SonarCloud Quality Gate failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to agree this is a problem (I don't think it is) before we solve it with this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to check if the select list is enabled. If it is disabled, the option will also be disabled. I'd prefer to optimize the number of commands made for a successful test rather than an unsuccessful test. What do you think?
Yes, I also like to make as few commands as possible. If |
A non-disabled That said, it's only one extra command when using this wrapper, and it does give a better error message, so maybe I'm being too picky. @diemol what do you think of this implementation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @asolntsev!
SonarCloud Quality Gate failed. |
fix for #10812