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

EnumListParameter #2801

Merged
merged 3 commits into from
Jan 16, 2020
Merged

EnumListParameter #2801

merged 3 commits into from
Jan 16, 2020

Conversation

evictor
Copy link
Contributor

@evictor evictor commented Oct 9, 2019

Description

A parameter type complementary to EnumParameter that allows for an arbitrarily sized list (0 to n) of enum values in a str-delimited format.

Motivation and Context

Convenient addition to the Luigi parameter type inventory.

Have you tested this? If so, how?

I have included unit tests that match the style of existing EnumParameter tests.

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

Looks really good! Thanks for adding this. Can you just remove the delim? I see that 1) the name delim sounds a bit sporadic is 2) untested and 3) undocumented. Only add code paths you have the time to give proper love too. :)

Or do you actually need a delim for your use case?

luigi/parameter.py Show resolved Hide resolved
@evictor
Copy link
Contributor Author

evictor commented Oct 15, 2019

OK, I removed the delim param and renamed the underlying variable to sep to follow Python's underlying var name for the similar concept.

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

Looks good. Please take a look at my comments, then we should merge this.

luigi/parameter.py Show resolved Hide resolved
luigi/parameter.py Show resolved Hide resolved
test/parameter_test.py Show resolved Hide resolved
@Tarrasch Tarrasch merged commit 57c32de into spotify:master Jan 16, 2020
@Tarrasch
Copy link
Contributor

Travis for 2.7 was red, but it's gone now. :D

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