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

Do not warn about missing param if there is a default #13

Draft
wants to merge 1 commit into
base: fs-2.7.8
Choose a base branch
from

Conversation

jonshea
Copy link

@jonshea jonshea commented Aug 10, 2023

Our logs are spammed with warnings like

UserWarning: Parameter "graphite_host" with value "None" is not of type string.

usually for parameters that have a default set. I would like to disable this warning if there is a default set.

Our logs are spammed with warnings like
```
UserWarning: Parameter "graphite_host" with value "None" is not of type string.
```
usually for parameters that have a default set. I would like to disable this
warning if there is a default set.
@jonshea jonshea requested review from a team August 10, 2023 18:12
@jonshea jonshea marked this pull request as ready for review August 10, 2023 20:06
@@ -343,7 +343,7 @@ def parse(self, x):
def _warn_on_wrong_param_type(self, param_name, param_value):
if self.__class__ != OptionalParameter:
return
if not isinstance(param_value, six.string_types) and param_value is not None:
if not isinstance(param_value, six.string_types) and not (param_value is None and self._default != _no_value):

Choose a reason for hiding this comment

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

could this type of parameter have a default value but get passed a None, which might now cause this to complain?

Copy link
Author

Choose a reason for hiding this comment

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

I’m not sure I follow. I don’t think this change can make new complaints. But are you saying that this is OptionalParameter, so it must have a default, so we shouldn’t even bother with the and self._default != _no_value, because None is always a valid value?

Choose a reason for hiding this comment

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

If you had task SomeTask with
some_param = luigi.OptionalParameter(default="a string")
couldn't this complain if you instantiated it as SomeTask(some_param=None)?
i may be incorrect in my assumptions about where this gets applied.

@jonshea jonshea requested a review from OniOni August 14, 2023 17:01
@jonshea jonshea marked this pull request as draft December 1, 2023 17:01
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.

None yet

2 participants