[contrib.hdfs.target.HdfsFlagTarget] fixed argument forwarding to the… #2634
Conversation
luigi/contrib/hdfs/target.py
Outdated
| @@ -214,7 +214,7 @@ def __init__(self, path, format=None, client=None, flag='_SUCCESS'): | |||
| if path[-1] != "/": | |||
| raise ValueError("HdfsFlagTarget requires the path to be to a " | |||
| "directory. It must end with a slash ( / ).") | |||
| super(HdfsFlagTarget, self).__init__(path, format, client) | |||
| super(HdfsFlagTarget, self).__init__(path, format, is_tmp=False, fs=client) | |||
There was a problem hiding this comment.
There is not need to set is_tmp=False which is the default value in HdfsTarget.
There was a problem hiding this comment.
I thought that for sake of human readability I better should name-mention parameters that seem being out of context / have different naming but if it doesn't fit, I'll change that
There was a problem hiding this comment.
I meant there is no need to pass in is_tmp, fs=client is sufficient.
|
Looks like a good fix. How come this was not caught by test case? It was introduced by this PR: #2559 |
|
@honnix the test case covers only |
|
It would be nicer we can add test case to cover the change. But I don't have strong opinion in this case. What do you say @dlstadther ? |
|
I'm not overly familiar with these HDFS classes. @dgronskij What is the naming convention which is broken? I agree with @honnix that test coverage would be great. I often rely on Travis tests in areas of Luigi to which i'm much less familiar. |
|
@dlstadther, well by "broken" I meant using BTW, it seems to me (please, correct me if it's not true, I have no previous experience with Travis CI / Nose) that HDFS related stuff is not tested at all: Line 31 in 55ca81d There is no notion of hdfs target tests in travis build log, too. How is it meant to be tested? Manual runs against forked repos (by the way, is it possible)? Moreover, UPD |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions. |
There is a mismatch between
HdfsTarget.__init__signature and how it's called from theHdfsFlagTarget.__init__