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

add a lint to warn on implicit string concatenation #7286

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Feb 26, 2019

Problem

It's occasionally possible to make mistakes when formatting strings using the "implicit" syntax of just placing two string literals next to each other. Although this pattern is used extremely commonly in the codebase right now, it would be nice to be able to point this out.

Solution

  • Add a linter for string concatenations of the form e.g. 'a' 'b'.
  • Add a suppressions file which turns it off for all python files in this repo.

Result

It's possible this could lead to fewer errors. However, there is currently a lot of noise induced by turning this on and it would be nice to avoid that by eventually fixing code to remove it (e.g. by using triple-quoted """ strings instead), but for now, it has been suppressed. Other repos can freely use this lint, however.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:ban-implicit-string-concatenation branch 2 times, most recently from 6022b32 to e587cb8 Mar 1, 2019

@cosmicexplorer cosmicexplorer changed the title add a lint to warn on implicit string concatenation add a (commented out) lint to warn on implicit string concatenation Mar 3, 2019

cosmicexplorer added some commits Feb 26, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:ban-implicit-string-concatenation branch from 16216e3 to 933c43d Mar 4, 2019

@cosmicexplorer cosmicexplorer changed the title add a (commented out) lint to warn on implicit string concatenation add a (suppressed, in this repo) lint to warn on implicit string concatenation Mar 4, 2019

@cosmicexplorer cosmicexplorer changed the title add a (suppressed, in this repo) lint to warn on implicit string concatenation add a lint to warn on implicit string concatenation Mar 4, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Added a suppressions file to turn it off in this repo!

@stuhood

stuhood approved these changes Mar 4, 2019

Copy link
Member

left a comment

Thanks!

@cosmicexplorer cosmicexplorer merged commit 3a81e86 into pantsbuild:master Mar 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Mar 8, 2019

cosmicexplorer added a commit that referenced this pull request Mar 8, 2019

Revert "add a lint to warn on implicit string concatenation (#7286)" (#…
…7340)

This reverts commit 3a81e86.

### Problem

The use of the `asttokens` library, now within the pythonstyle checker pex, is causing a variant of #7158 when consumed outside of this repo. Additionally, some files fail to parse with:
```
    for tok in tokenize.generate_tokens(StringIO(str_node_text).readline):
  File "/opt/ee/python/2.7/lib/python2.7/tokenize.py", line 374, in generate_tokens
    ("<tokenize>", lnum, pos, line))
  File "<tokenize>", line 3
    "else echo 'no_op'; fi)"
```

Suppressing the lint only works if the lint runs without error, so this makes it impossible to consume outside of the pants repo right now (or at least, impossible in a lot of repos outside of the pants repo).

### Solution

- Revert #7286.

### Result

This will be unreverted asap when the above issues are investigated and fixed.

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Mar 14, 2019

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Mar 14, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Hey Danny, could you please elaborate a bit more on the motivation here? Is this only for implicit string concatenation when done on the same line or for multiline strings too?

For example, would this still be valid?

print("My really long string that I want to all appear "
         "on the same line when printed but I don't want to exceed "
         "100 columns in the editor.")
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

No, see the test added in #7377:

def test_handles_inconsistent_indentation(self):
multiline_multiple_indent_text = """\
("$(if [ -e ./{0} -a -e ./{1} ]; then echo 'mark_success'; "
"elif [ -e ./{1} ]; then echo 'mark_failed'; "
"else echo 'no_op'; fi)")"""
self.assertNit(multiline_multiple_indent_text, 'T806', Nit.WARNING)

This is why that PR is blocked by #7378, because as mentioned in that PR it does not embody a typical approach to strings in python -- this is why it is a warning instead of an error. It is a feature that a nonzero number of users would find useful, and it is an avenue (via #7378) for things like suppressing lints by default across the whole repo instead of assuming all users will always want all lints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.