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

add ChoiceListParameter #3305

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

kitagry
Copy link
Contributor

@kitagry kitagry commented Sep 2, 2024

Description

A parameter type complementary to ChoiceParameter that allows for an arbitrarily sized list (0 to n).

Motivation and Context

Convenient addition to the Luigi parameter type inventory.

Have you tested this? If so, how?

I have included unit tests.

@kitagry kitagry marked this pull request as ready for review September 2, 2024 13:21
@kitagry kitagry requested review from dlstadther and a team as code owners September 2, 2024 13:21
import luigi


class ChoiceListParameterTest(unittest.TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests should be put into test/parameter_test.py

@@ -1540,6 +1540,66 @@ def normalize(self, var):
var=var, choices=self._choices))


class ChoiceListParameter(Parameter):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine with me with one exception. If this class is intended to be a multi-select of ChoiceParameter, it's counter to object-oriented for it to not inherit from ChoiceParameter (or a common Choice-based parent) such for them to maintain alignment of their choice parsing and validation.

What are your thoughts?

@kitagry
Copy link
Contributor Author

kitagry commented Sep 16, 2024

@dlstadther Thank you for the review! and I'm sorry for the late response.

I fixed test and ListChoiceParameter inheritance.

@dlstadther
Copy link
Collaborator

@kitagry , could you take a look at the failing tests? I've restarted those which failed to check on potentially flakey tests (i know there are a couple luigi tests which sometimes fail and sometimes pass).

@dlstadther dlstadther merged commit 8804843 into spotify:master Sep 24, 2024
48 checks passed
@kitagry kitagry deleted the add-choices-list-parameter branch September 26, 2024 00:23
@kitagry
Copy link
Contributor Author

kitagry commented Sep 26, 2024

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants