Skip to content

Conversation

@AtR1an
Copy link
Contributor

@AtR1an AtR1an commented Jan 2, 2026

Allows to dynamically filter which enum values are displayed.

@AtR1an AtR1an requested a review from a team as a code owner January 2, 2026 13:54
Copilot AI review requested due to automatic review settings January 2, 2026 13:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds dynamic filtering capabilities to EnumParameter through a new visible_choices parameter. This allows enum options to be filtered based on runtime context (e.g., input data specifications) while maintaining backward compatibility for saved workflows.

Key changes:

  • Added visible_choices callable parameter to filter displayed enum options based on DialogCreationContext
  • Enhanced EnumParameter to accept enum members directly as default_value (not just strings)
  • Implemented caching (LRU with size 2) for the filtering callable to optimize performance

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
org.knime.python3.nodes/src/main/python/knime/extension/parameter.py Core implementation: added visible_choices parameter, filtering logic with validation/warnings, caching, and comprehensive docstring examples
org.knime.python3.nodes.tests/src/test/python/unittest/test_knime_parameter.py Comprehensive test suite covering filtering scenarios, edge cases (empty/invalid results), caching behavior, and description generation

Comment on lines 1519 to 1525
Parameters
----------
docstring : str
The parameter docstring
visible_members : list, optional
List of member names to include. If None, all members are included.
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The docstring format is inconsistent with the method's return value. The docstring should include a Returns section describing that it returns a string containing the formatted options description.

Suggested change
Parameters
----------
docstring : str
The parameter docstring
visible_members : list, optional
List of member names to include. If None, all members are included.
Parameters
----------
docstring : str
The parameter docstring.
visible_members : list, optional
List of member names to include. If None, all members are included.
Returns
-------
str
The formatted options description including the available options.

Copilot uses AI. Check for mistakes.
Comment on lines 1766 to 1781
LOGGER.warning(
f"visible_choices for parameter '{self._label}' returned an empty list. "
f"Showing empty options."
)
return []
else:
# All members were invalid - already warned above
LOGGER.warning(
f"visible_choices for parameter '{self._label}' returned no valid members. "
f"Showing empty options."
)
return []
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

Both branches of this conditional return the same value (empty list) and log similar warnings. The code can be simplified by removing the nested conditional since both cases should be handled identically - check if validated_members is empty, log an appropriate warning mentioning whether it was originally empty or had only invalid members, and return an empty list.

Suggested change
LOGGER.warning(
f"visible_choices for parameter '{self._label}' returned an empty list. "
f"Showing empty options."
)
return []
else:
# All members were invalid - already warned above
LOGGER.warning(
f"visible_choices for parameter '{self._label}' returned no valid members. "
f"Showing empty options."
)
return []
warning_msg = (
f"visible_choices for parameter '{self._label}' returned an empty list. "
f"Showing empty options."
)
else:
# All members were invalid - already warned above
warning_msg = (
f"visible_choices for parameter '{self._label}' returned no valid members. "
f"Showing empty options."
)
LOGGER.warning(warning_msg)
return []

Copilot uses AI. Check for mistakes.
@AtR1an AtR1an force-pushed the enh/AP-25288-structured-outputs branch from 50d6697 to f7d643c Compare January 5, 2026 09:40
@chaubold
Copy link
Contributor

Nice additional feature for enum displays in Python nodes! One thing that I stumbled upon multiple times when reading is that it "shows all items if the value for visible_items is None".

Why don't we add a hidden_items method that does the opposite, but then the None semantics are much clearer: if no hidden specified, show all items?

@AtR1an
Copy link
Contributor Author

AtR1an commented Jan 12, 2026

Nice additional feature for enum displays in Python nodes! One thing that I stumbled upon multiple times when reading is that it "shows all items if the value for visible_items is None".

Why don't we add a hidden_items method that does the opposite, but then the None semantics are much clearer: if no hidden specified, show all items?

@copilot can you implement this variant?

Copy link
Contributor

Copilot AI commented Jan 12, 2026

@AtR1an I've opened a new pull request, #41, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI review requested due to automatic review settings January 12, 2026 15:17
@chaubold chaubold force-pushed the enh/AP-25288-structured-outputs branch from 67c4c24 to 2ae5818 Compare January 12, 2026 15:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

org.knime.python3.nodes/src/main/python/knime/extension/parameter.py:1

  • The example code has incorrect indentation. Line 1630 should not have the ... prefix and should be dedented to align with the class definition. The function hide_by_model_support should be defined at module level, not as a continuation of the class definition.
# -*- coding: utf-8 -*-

@chaubold chaubold force-pushed the enh/AP-25288-structured-outputs branch from 2ae5818 to 8a5a476 Compare January 12, 2026 15:22
Copilot AI review requested due to automatic review settings January 12, 2026 15:44
@chaubold chaubold force-pushed the enh/AP-25288-structured-outputs branch from 8a5a476 to 66c7036 Compare January 12, 2026 15:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment on lines 1931 to 1933
# Should be called once: the same context is used for both description and schema
# generation, and the result is cached after the first call
self.assertEqual(call_count[0], 1)
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The test comment claims the callable should be called once due to caching, but extract_schema is called three times with the same context. The assertion expects call_count[0] to be 1, which would only be true if caching works perfectly. However, _extract_description and _extract_schema may call _get_visible_options separately, potentially resulting in more than one call. Verify that both methods actually share the cached result, or adjust the expected call count.

Suggested change
# Should be called once: the same context is used for both description and schema
# generation, and the result is cached after the first call
self.assertEqual(call_count[0], 1)
# The hidden_choices callable should not be called more often than extract_schema
# is invoked with the same context (caching may further reduce this count).
self.assertLessEqual(call_count[0], 3)

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings January 13, 2026 09:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings January 13, 2026 09:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings January 13, 2026 09:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@AtR1an AtR1an force-pushed the enh/AP-25288-structured-outputs branch from c68f4ea to 7efe31e Compare January 13, 2026 14:57
@sonarqubecloud
Copy link

@AtR1an AtR1an merged commit 1a3b60f into master Jan 13, 2026
3 of 4 checks passed
@AtR1an AtR1an deleted the enh/AP-25288-structured-outputs branch January 13, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants