Fix detection of binary open() calls in the Python encoding linter
ClosedPublic
Actions

Authored by jasonbcox on Sep 2 2020, 21:14.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC77c3f9dc3ce4: Fix detection of binary open() calls in the Python encoding linter

Summary

Currently, the python file encoding linter incorrectly identifies some cases of open() needing an encoding explicitly set when they in fact do not.
Reading/writing binary files should not set an encoding, so this false positive lint message will lead the developer to produce incorrect code
if the message is to be believed.

This patch fixes the pattern matching for binary-mode open() calls, which will allow the buildbot code to be ported to this repository
without breaking the linter.

Test Plan

arc lint --everything

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Sep 2 2020, 21:14

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 2 2020, 21:14

jasonbcox requested review of this revision.Sep 2 2020, 21:14

Harbormaster completed remote builds in B12571: Diff 23212.Sep 2 2020, 21:29

jasonbcox added a child revision: D7328: Port the abcbot code into this repository.Sep 2 2020, 21:36

I have no idea what problem this fixes.

Sure, this match different things. Ok. Why should it match these things? What problem happens when it doesn't?

This revision now requires changes to proceed.Sep 2 2020, 23:39

In D7326#175088, @deadalnix wrote:

I have no idea what problem this fixes.

Sure, this match different things. Ok. Why should it match these things? What problem happens when it doesn't?

[^,]*, Only matches one comma, so calls to open() where the first arg is composed of other calls (in the above example, it's os.path.join(os.path.dirname(__file__), 'resources', 'teamcity-icon-16.base64')), the match is incorrect.
I misread the [^'\"] and should have removed it. Will fix. The new matching intends to restrict the file mode matching so that it doesn't just match the first string it finds that contains a b.

Fix restricting file mode matching

Harbormaster completed remote builds in B12586: Diff 23236.Sep 3 2020, 15:49

In D7326#175189, @jasonbcox wrote:

In D7326#175088, @deadalnix wrote:

I have no idea what problem this fixes.

Sure, this match different things. Ok. Why should it match these things? What problem happens when it doesn't?

[^,]*, Only matches one comma, so calls to open() where the first arg is composed of other calls (in the above example, it's os.path.join(os.path.dirname(__file__), 'resources', 'teamcity-icon-16.base64')), the match is incorrect.

I misread the [^'\"] and should have removed it. Will fix. The new matching intends to restrict the file mode matching so that it doesn't just match the first string it finds that contains a b.

You are explaining the code to me. I do not care about the code, I want to know what the problem is with this code. This needs to be part of the description.

If not for me, then for whoever is going to dig in the history of that whole thing trying to figure out WTF is going on.

This revision now requires changes to proceed.Sep 3 2020, 18:11

Completely rewrote the summary to focus on the problem being solved. The code change mostly speaks for itself.

jasonbcox requested review of this revision.Sep 3 2020, 18:38

deadalnix accepted this revision.Sep 3 2020, 21:37

This revision is now accepted and ready to land.Sep 3 2020, 21:37

Closed by commit rABC77c3f9dc3ce4: Fix detection of binary open() calls in the Python encoding linter (authored by jasonbcox). · Explain WhySep 3 2020, 22:05

This revision was automatically updated to reflect the committed changes.

jasonbcox added a commit: rABC77c3f9dc3ce4: Fix detection of binary open() calls in the Python encoding linter.