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

Make luigi package more flake8 compliant #1175

Merged
merged 1 commit into from
Aug 31, 2015
Merged

Make luigi package more flake8 compliant #1175

merged 1 commit into from
Aug 31, 2015

Conversation

ediskandarov
Copy link
Contributor

Motivation of this PR was to make flake8 checks as strict as possible.

Before:

commands = flake8 --max-line-length=384 --exclude=doc,test,luigi/six.py
  flake8 --max-line-length=100 --ignore=E265 doc
  flake8 --max-line-length=252 --ignore=F401,F841 test

After:

commands = flake8 --max-line-length=160 --exclude=doc,luigi/six.py
   flake8 --max-line-length=100 --ignore=E265 doc

This will protect luigi package from govnokod.

In future max-line-length should be less then 120 chars for better readability.

@@ -97,7 +101,8 @@ def _get_str(task_dict, extra_indent):
break
if len(tasks[0].get_params()) == 0:
row += '- {0} {1}()'.format(len(tasks), str(task_family))
elif _get_len_of_params(tasks[0]) > 60 or (len(tasks) == 2 and len(tasks[0].get_params()) > 1 and (_get_len_of_params(tasks[0]) > 40 or len(str(tasks[0])) > 100)) or len(str(tasks[0])) > 200:
elif _get_len_of_params(tasks[0]) > 60 or len(str(tasks[0])) > 200 or \
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought \ were unpythonic and one preferred parenthesis? No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess there is no more or less pythonic. In my opinion - pythonic mean better readable.

In some cases parenthesis better readable but in this case - this condition \ looks good. Condition must no be over-parenthesised.

https://www.python.org/dev/peps/pep-0008/#maximum-line-length

Backslashes may still be appropriate at times. For example, long, multiple with -statements cannot use implicit continuation, so backslashes are acceptable:

@Tarrasch
Copy link
Contributor

Awesome progress, for future reference, this a continuation of #1174.

@Tarrasch
Copy link
Contributor

Thanks so much for caring about the code quality of luigi! :)

@Tarrasch
Copy link
Contributor

What's "govnokod" by the way? I only found results in Russian. :)

@ediskandarov
Copy link
Contributor Author

Thanks so much for caring about the code quality of luigi! :)

We use luigi and loved it. I wish bright future for project. You are welcome. Thank you for your work too.

What's "govnokod" by the way?

Well.. It is very specific term. Usually govnokod generated by anything but not developers head. It is like monkey code but a little bit different.

Talk is cheap. I show you the code: http://govnokod.ru :-))

' - other_worker ran 1 tasks\n\n'
'Did not run any tasks\n'
'This progress looks :) because there were no failed '
'tasks or missing external dependencies\n', s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this looks so much nicer! :)

Tarrasch added a commit that referenced this pull request Aug 31, 2015
Make luigi package more flake8 compliant
@Tarrasch Tarrasch merged commit 7b0d98a into spotify:master Aug 31, 2015
@Tarrasch
Copy link
Contributor

This is so awesome! Thanks :)

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