AP-25288: Replace visible_choices with hidden_choices for EnumParameter #41
Conversation
Co-authored-by: AtR1an <12174376+AtR1an@users.noreply.github.com>
Co-authored-by: AtR1an <12174376+AtR1an@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| self.assertIn("Valid options", warning_msg) | ||
|
|
||
| # Schema should only contain valid member | ||
| # Schema should hide only the valid member (LINEAR) |
There was a problem hiding this comment.
the invalid member LINEAR?
There was a problem hiding this comment.
Clarified in da43ba1. LINEAR is a valid enum member that's being hidden (the only valid member in the list that also contains invalid members). The test verifies that invalid members are filtered out with a warning, while valid members like LINEAR are properly hidden.
|
|
||
| # Validate that all returned members exist in the enum | ||
| valid_names = set(self._enum._member_names_) | ||
| validated_members = [] |
There was a problem hiding this comment.
this should probably still be validated_members instead of validated_hidden and invalid_members
There was a problem hiding this comment.
Fixed in da43ba1. Renamed validated_hidden to validated_members for consistency with the codebase pattern.
| ------- | ||
| str | ||
| A formatted description string containing the available options, | ||
| optionally restricted to the provided ``visible_options``. |
There was a problem hiding this comment.
visible_options should also be turned to hidden_options, right?
There was a problem hiding this comment.
The parameter name visible_options is semantically correct here. It contains the list of options that are visible (i.e., NOT hidden) after filtering. The method uses this list to generate a description showing those visible options. Renaming it to hidden_options would be misleading since it doesn't contain hidden options.
Co-authored-by: AtR1an <12174376+AtR1an@users.noreply.github.com>
Addressed @chaubold's comments in da43ba1. Fixed variable naming and clarified test comment. Regarding the |
|
@AtR1an I like the changes. If you agree, feel free to merge |
Implementation Plan: Replace visible_choices with hidden_choices
visible_choicesparameter tohidden_choices_generate_options_description_generate_descriptionvalidated_hiddentovalidated_membersfor consistency💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.