Page MenuHomePhabricator

Add a linter to detect python % format and convert them to use .format()
ClosedPublic

Authored by Fabien on Feb 15 2019, 20:25.

Details

Summary

Given a python script at input, the linter will output a list of the
code snippets embedding a % format and suggest .format replacement for
them.
If called with no argument, the linter will run the doctests to check
its behavior.
This is intended to be integrated into arcanist in a later diff as an
autofix linter.

Note: the linter suggestion will transform multiline code into a single
line.

Current limitation (would require a tokenizer):

  • This linter does not support inlined comments in the middle of a formatting (see last test case from test-python-format.txt for an example). This case should be handled manually.
Test Plan

Should return no error (no output, return code 0):

./test/lint/lint-python-format.py

Should return 31 errors:

./test/lint/lint-python-format.py

test/functional/test_framework/messages.py
Check the errors are not false-positive and that the suggestions are
correct.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
lint_python_format
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5009
Build 8081: Bitcoin ABC Buildbot (legacy)
Build 8080: arc lint + arc unit

Event Timeline

I trust that you know what you are doing. It comes with tests and all, and worst case scenario, we can fix it.

This revision is now accepted and ready to land.Feb 16 2019, 00:51

Add a missing case when line ends with % and is not followed by \

Fabien edited the summary of this revision. (Show Details)

Found another limitation, add a commented test case in the txt file to reflect it

Fabien planned changes to this revision.Feb 18 2019, 11:53

The second limitation is an important part of the use case. It needs to be handled correctly.

More special cases handling:

  • Allow nested formatting in functions, lists or dictionaries
  • Allow multiple formatting on a single line
  • Handle special cases for %i (no longer exist), %s (more strict behavior), %d, %r and %-
  • Replace %% with %
This revision is now accepted and ready to land.Feb 18 2019, 21:06
jasonbcox requested changes to this revision.Feb 20 2019, 00:40
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/lint/lint-python-format-tests.txt
58 ↗(On Diff #7384)

Needs more negative tests, such as:

# Strings with % but not using it for formatting
"test %100"
"test %\n"
"test %\
    1"
This revision now requires changes to proceed.Feb 20 2019, 00:40

Add negative test cases as per feedback

jasonbcox requested changes to this revision.Feb 21 2019, 21:55
jasonbcox added inline comments.
test/lint/lint-python-format-tests.txt
70 ↗(On Diff #7418)

Isn't this supposed to be wrapped in double quotes?

This revision now requires changes to proceed.Feb 21 2019, 21:55
Fabien requested review of this revision.Feb 22 2019, 07:17
Fabien added inline comments.
test/lint/lint-python-format-tests.txt
70 ↗(On Diff #7418)

I added it to ensure that a modulus operator (thus no quote) is not detected as a formatting token.
There was no test to check that it is correctly handled by the linter.

This revision is now accepted and ready to land.Feb 28 2019, 22:19
This revision was automatically updated to reflect the committed changes.