teemtee / fmf

Flexible Metadata Format
GNU General Public License v2.0
22 stars 28 forks source link

Add pre-commit to fix formatting and python rules #130

Closed jscotka closed 3 years ago

jscotka commented 3 years ago

ideally in another PR fix all other issues (pre-commit run --all):

tests/unit/test_modify.py:37:9: F841 local variable 'item_parent' is assigned to but never used
fmf/utils.py:16:1: F401 'pprint.pformat as pretty' imported but unused
fmf/utils.py:644:73: F841 local variable 'lock' is assigned to but never used
fmf/utils.py:850:26: F821 undefined name 'unicode'
fmf/utils.py:881:9: E731 do not assign a lambda expression, use a def
tests/unit/test_adjust.py:152:39: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_adjust.py:160:39: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_adjust.py:167:39: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_adjust.py:174:38: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_adjust.py:182:38: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_adjust.py:190:38: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_adjust.py:198:38: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_context.py:68:9: F841 local variable 'fedora' is assigned to but never used
tests/unit/test_context.py:104:9: F841 local variable 'bar_800' is assigned to but never used
tests/unit/test_context.py:106:9: F841 local variable 'bar_8' is assigned to but never used
tests/unit/test_context.py:148:9: F841 local variable 'bar_800' is assigned to but never used
tests/unit/test_context.py:198:9: F841 local variable 'mix' is assigned to but never used
fmf/base.py:42:1: E302 expected 2 blank lines, found 1
fmf/base.py:134:25: E711 comparison to None should be 'if cond is not None:'
fmf/base.py:566:13: F841 local variable 'name' is assigned to but never used
fmf/base.py:567:13: F841 local variable 'data' is assigned to but never used
fmf/base.py:568:13: F841 local variable 'root' is assigned to but never used
fmf/base.py:569:13: F841 local variable 'sources' is assigned to but never used
tests/unit/test_utils.py:40:48: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:41:49: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:42:54: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:43:59: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:44:48: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:45:49: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:46:58: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:47:55: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:51:61: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:52:63: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:53:63: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:54:71: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:55:63: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:56:67: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:57:61: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:58:61: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:59:61: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:60:71: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:61:71: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:65:55: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:66:57: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:67:49: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:68:57: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:69:57: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:70:55: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:74:62: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:75:65: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:76:64: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:77:63: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:81:65: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:82:67: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:86:47: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:87:46: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
tests/unit/test_utils.py:88:53: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:89:54: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_utils.py:162:9: F841 local variable 'text' is assigned to but never used
tests/unit/test_utils.py:279:13: F841 local variable 'repo' is assigned to but never used
tests/unit/test_utils.py:313:17: F841 local variable 'repo' is assigned to but never used
tests/unit/test_base.py:94:13: F841 local variable 'tree' is assigned to but never used
tests/unit/test_base.py:117:39: E712 comparison to True should be 'if cond is True:' or 'if cond:'
tests/unit/test_base.py:166:47: E711 comparison to None should be 'if cond is None:'
tests/unit/test_base.py:182:13: F841 local variable 'tree' is assigned to but never used
tests/unit/test_base.py:326:17: F841 local variable 'node' is assigned to but never used
psss commented 3 years ago

Why does the check convert single quotes to double quotes? What's wrong with the single quote style strings?

jscotka commented 3 years ago

Why does the check convert single quotes to double quotes? What's wrong with the single quote style strings?

I do not have idea, but probably it is more pythonish :-) or there is some python best practice rule to use " when possible and not '

thrix commented 3 years ago

yeah, black is opinionated, on the other hand one does not care about style problems anymore :(

FrNecas commented 3 years ago

When it comes down to ' vs " it's just preference (even according to PEP8). Either is fine but one should be consistent and use the same type across the code (with the exception of using quotes inside the string, then you may use the other type to avoid escaping the quotes). We use them rather inconsistently in tmt/fmf :/ Black apparently forces double quotes

jscotka commented 3 years ago

yeah, black is opinionated, on the other hand one does not care about style problems anymore :(

Right, and it is what I like, why to care about formatting, when there exist tooling like autopep, black or similar tools. And finally it also helps you to understand these rules, because you see therese changes. connection with pre-commit helps you to focus on coding, not this bureaucracy :-)

psss commented 3 years ago

I don't think this is about bureaucracy, but readability counts. It's very true that we read the code much more many times than writing it. So it makes a perfect sense to me to have a beautiful code ;-) I've experimented a bit with black and ran into a couple important problems which did not convince me about the benefits.

psss commented 3 years ago

Let's make the pre-commit setup consistent with tmt. I'll re-use this pull request for that.

psss commented 3 years ago

Need a non-fork pull request to test the pre-commit config directly. Covered by #137.