Page MenuHomePhabricator
Differential D2557

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

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

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING3197e88a76af: Add a linter to detect python % format and convert them to use .format()
rABC3197e88a76af: Add a linter to detect python % format and convert them to use .format()
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien created this revision.Feb 15 2019, 20:25
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 15 2019, 20:25
Herald added a reviewer: deadalnix. · View Herald Transcript
Herald added a subscriber: schancel. · View Herald Transcript
teamcity added a subscriber: teamcity.Feb 15 2019, 20:26

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien added a child revision: D2558: Integrates the python format linter with arcanist.Feb 15 2019, 20:36
teamcity added a comment.Feb 15 2019, 20:40

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B4999: Diff 7361!Feb 15 2019, 20:40
teamcity added a comment.Feb 15 2019, 20:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 15 2019, 20:59

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4999: Diff 7361.Feb 15 2019, 20:59
Fabien updated this revision to Diff 7363.Feb 15 2019, 21:06

Rebase

teamcity added a comment.Feb 15 2019, 21:07

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 15 2019, 21:21

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5001: Diff 7363.Feb 15 2019, 21:21
deadalnix accepted this revision.Feb 16 2019, 00:51

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
Fabien updated this revision to Diff 7372.Feb 18 2019, 09:54

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

teamcity added a comment.Feb 18 2019, 09:55

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 18 2019, 10:09

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5009: Diff 7372.Feb 18 2019, 10:09
Fabien edited the summary of this revision. (Show Details)Feb 18 2019, 10:45
Fabien edited the summary of this revision. (Show Details)
Fabien updated this revision to Diff 7373.Feb 18 2019, 10:54

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

teamcity added a comment.Feb 18 2019, 10:55

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 18 2019, 11:09

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5010: Diff 7373.Feb 18 2019, 11:09
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.

Fabien updated this revision to Diff 7384.Feb 18 2019, 21:06

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
Fabien edited the summary of this revision. (Show Details)Feb 18 2019, 21:06
teamcity added a comment.Feb 18 2019, 21:07

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5016: Diff 7384.Feb 18 2019, 21:20
teamcity added a comment.Feb 18 2019, 21:20

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

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
Fabien updated this revision to Diff 7418.Feb 20 2019, 08:19

Add negative test cases as per feedback

teamcity added a comment.Feb 20 2019, 08:21

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 20 2019, 08:35

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5035: Diff 7418.Feb 20 2019, 08:35
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.

jasonbcox accepted this revision.Feb 28 2019, 22:19
This revision is now accepted and ready to land.Feb 28 2019, 22:19
Closed by commit rABC3197e88a76af: Add a linter to detect python % format and convert them to use .format() (authored by Fabien). · Explain WhyMar 1 2019, 13:34
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rSTAGING3197e88a76af: Add a linter to detect python % format and convert them to use .format().Sep 18 2019, 22:36

Revision Contents
Changeset List

PathSize
test/
lint/
70 lines
266 lines

Diff 7554

View Options

test/lint/lint-python-format-tests.txt

Loading...
View Options

test/lint/lint-python-format.py

Loading...