Enforce ruff/flake8-implicit-str-concat rules (ISC) #4411

DimitriPapadopoulos · 2024-06-02T22:28:07Z

Summary of changes

ISC001 Implicitly concatenated string literals on one line

ISC003 Explicitly concatenated string should be implicitly concatenated

Pull Request Checklist

Changes have tests
News fragment added in newsfragments/.
(See documentation for details)

DimitriPapadopoulos · 2024-06-02T22:55:09Z

pkg_resources/tests/test_resources.py

@@ -257,7 +257,7 @@ def test_marker_evaluation_with_extras(self):
 "/foo_dir/Foo-1.2.dist-info",
 metadata=Metadata((
 "METADATA",
- "Provides-Extra: baz\n" "Requires-Dist: quux; extra=='baz'",
+ "Provides-Extra: baz\nRequires-Dist: quux; extra=='baz'",


Use triple-quoted strings instead?

I don't think it is necessary to use triple-quotes here, specially in the context of quick tests...

But then tests are as important and should be as maintainable as the source code.

I don't think that adding an explicit \n makes short snippets unreadable.
For tests, it even makes sense, because we are explicitly saying there should be a \n there...

On the other hand, using triple quotes for short strings either results in distracting code (e.g. when the indentation is broken inside the triple-quotes), or requires you to bring out the big guns (e.g. inspect.cleandoc).

DimitriPapadopoulos · 2024-06-02T22:55:38Z

setuptools/command/bdist_egg.py

 'getsourcefile',
- 'getfile' 'getsourcelines',
+ 'getsourcelines',


That was a real bug!

Thank you very much @DimitriPapadopoulos for working on this.

Given that this PR for the time being is failing the CI, would you like to extract the bug fixes in a PR of their own?

I can do that, but what's the intent? As far as I can tell, this change is not (directly) responsible for the CI failures.

Great, if the CI is no longer failure that is OK. We can proceed with this PR.

DimitriPapadopoulos · 2024-06-02T22:56:09Z

setuptools/tests/config/test_expand.py

@@ -83,7 +83,7 @@ def test_read_attr(self, tmp_path, monkeypatch):
 "pkg/__init__.py": "",
 "pkg/sub/__init__.py": "VERSION = '0.1.1'",
 "pkg/sub/mod.py": (
- "VALUES = {'a': 0, 'b': {42}, 'c': (0, 1, 1)}\n" "raise SystemExit(1)"
+ "VALUES = {'a': 0, 'b': {42}, 'c': (0, 1, 1)}\nraise SystemExit(1)"


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:56:22Z

setuptools/tests/config/test_setupcfg.py

@@ -35,7 +35,7 @@ def fake_env(
 tmpdir, setup_cfg, setup_py=None, encoding='ascii', package_path='fake_package'
 ):
 if setup_py is None:
- setup_py = 'from setuptools import setup\n' 'setup()\n'
+ setup_py = 'from setuptools import setup\nsetup()\n'


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:56:29Z

setuptools/tests/config/test_setupcfg.py

@@ -97,7 +97,7 @@ def test_no_config(self, tmpdir):
 def test_ignore_errors(self, tmpdir):
 _, config = fake_env(
 tmpdir,
- '[metadata]\n' 'version = attr: none.VERSION\n' 'keywords = one, two\n',
+ '[metadata]\nversion = attr: none.VERSION\nkeywords = one, two\n',


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:56:34Z

setuptools/tests/config/test_setupcfg.py

@@ -171,22 +171,22 @@ def test_license_cfg(self, tmpdir):
 def test_file_mixed(self, tmpdir):
 fake_env(
 tmpdir,
- '[metadata]\n' 'long_description = file: README.rst, CHANGES.rst\n' '\n',
+ '[metadata]\nlong_description = file: README.rst, CHANGES.rst\n\n',


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:56:46Z

setuptools/tests/config/test_setupcfg.py

 )

 tmpdir.join('README.rst').write('readme contents\nline2')
 tmpdir.join('CHANGES.rst').write('changelog contents\nand stuff')

 with get_dist(tmpdir) as dist:
 assert dist.metadata.long_description == (
- 'readme contents\nline2\n' 'changelog contents\nand stuff'
+ 'readme contents\nline2\nchangelog contents\nand stuff'


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:56:52Z

setuptools/tests/config/test_setupcfg.py

 )

 def test_file_sandboxed(self, tmpdir):
 tmpdir.ensure("README")
 project = tmpdir.join('depth1', 'depth2')
 project.ensure(dir=True)
- fake_env(project, '[metadata]\n' 'long_description = file: ../../README\n')
+ fake_env(project, '[metadata]\nlong_description = file: ../../README\n')


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:56:59Z

setuptools/tests/config/test_setupcfg.py

@@ -253,7 +253,7 @@ def test_dict(self, tmpdir):

 def test_version(self, tmpdir):
 package_dir, config = fake_env(
- tmpdir, '[metadata]\n' 'version = attr: fake_package.VERSION\n'
+ tmpdir, '[metadata]\nversion = attr: fake_package.VERSION\n'


Use triple-quoted strings instead?

DimitriPapadopoulos · 2024-06-02T22:57:27Z

setuptools/tests/test_find_packages.py

@@ -180,7 +180,8 @@ class TestFlatLayoutPackageFinder:
 [
 "pkg/__init__.py",
 "examples/__init__.py",
- "examples/file.py" "example/other_file.py",
+ "examples/file.py",
+ "example/other_file.py",


Again a real bug!

Avasam · 2024-06-05T16:45:51Z

ISC001 definitively as there's no valid reason to ISC on a single line. It just makes the line longer and is likely to accidentally create bugs (which you did catch!)

ISC002 vs ISC003 is more opinionated, so up to the maintainers. Personally I prefer disallowing multiline implicit string concatenation as it helps from a quick read understanding that two lines are not two parameters.

Also here's a feature request to control ISC003 with [lint.flake8-implicit-str-concat] allow-multiline (would be nice if we can just select ISC astral-sh/ruff#11582 )

DimitriPapadopoulos · 2024-06-17T12:51:38Z

About ISC003, I tend to agree with the Linux kernel coding style:

2) Breaking long lines and strings

[...]
The preferred limit on the length of a single line is 80 columns.
[...]
However, never break user-visible strings such as printk messages because that breaks the ability to grep for them.

I think it make sense to unfold user-visible strings, I seem to recall ruff does allow literal strings longer than the admissible line length, up to a larger limit at least.

ISC001 Implicitly concatenated string literals on one line Only fix actual bugs in this PR, at least until unrelated CI failures are fixed.

DimitriPapadopoulos · 2024-06-17T13:02:49Z

I've changed this PR to only fix actual bugs uncovered by ISC001.

abravalheri · 2024-06-17T13:54:46Z

About ISC003, I tend to agree with the Linux kernel coding style:

We should probably align with skeleton.

abravalheri · 2024-06-17T14:00:42Z

Thank you very much @DimitriPapadopoulos.

DimitriPapadopoulos · 2024-06-17T14:58:10Z

About ISC003, I tend to agree with the Linux kernel coding style:

We should probably align with skeleton.

It clearly a bad idea to ignore ISC001. I'll raise an issue over there.

DimitriPapadopoulos · 2024-06-17T15:13:16Z

My bad, I am actually the one who (temporarily) ignored ISC001 and ISC002 in skeleton. However, this is only a temporary counter-measure to work around potential Conflicting lint rules. At least the ISC001 rule should be reinstated, when astral-sh/ruff#8272 is addressed.

In the meantime, I think it makes sense to apply ISC001 and ISC002, even if the rules are not yet enabled in the linter configuration.

DimitriPapadopoulos force-pushed the ISC branch 2 times, most recently from 1f218d4 to 7309fbf Compare June 2, 2024 22:33

DimitriPapadopoulos commented Jun 2, 2024

View reviewed changes

DimitriPapadopoulos force-pushed the ISC branch from 7309fbf to 3b448ac Compare June 17, 2024 12:43

DimitriPapadopoulos force-pushed the ISC branch from 3b448ac to 1691c78 Compare June 17, 2024 12:52

Enforce ruff/flake8-implicit-str-concat rule ISC001

551364d

ISC001 Implicitly concatenated string literals on one line Only fix actual bugs in this PR, at least until unrelated CI failures are fixed.

DimitriPapadopoulos force-pushed the ISC branch from 1691c78 to 551364d Compare June 17, 2024 13:01

abravalheri added a commit that referenced this pull request Jun 17, 2024

Fix accidental implicit string concatenation (#4411)

8aa9855

abravalheri merged commit 551364d into pypa:main Jun 17, 2024
22 checks passed

DimitriPapadopoulos deleted the ISC branch June 17, 2024 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce ruff/flake8-implicit-str-concat rules (ISC) #4411

Enforce ruff/flake8-implicit-str-concat rules (ISC) #4411

DimitriPapadopoulos commented Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

abravalheri Jun 17, 2024 •

edited

Loading

DimitriPapadopoulos Jun 17, 2024

abravalheri Jun 17, 2024

DimitriPapadopoulos Jun 2, 2024

abravalheri Jun 17, 2024 •

edited

Loading

DimitriPapadopoulos Jun 17, 2024

abravalheri Jun 17, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

DimitriPapadopoulos Jun 2, 2024

Avasam commented Jun 5, 2024

DimitriPapadopoulos commented Jun 17, 2024

2) Breaking long lines and strings

DimitriPapadopoulos commented Jun 17, 2024

abravalheri commented Jun 17, 2024 •

edited

Loading

abravalheri commented Jun 17, 2024

DimitriPapadopoulos commented Jun 17, 2024

DimitriPapadopoulos commented Jun 17, 2024

Enforce ruff/flake8-implicit-str-concat rules (ISC) #4411

Enforce ruff/flake8-implicit-str-concat rules (ISC) #4411

Conversation

DimitriPapadopoulos commented Jun 2, 2024

Summary of changes

Pull Request Checklist

Choose a reason for hiding this comment

abravalheri Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avasam commented Jun 5, 2024

DimitriPapadopoulos commented Jun 17, 2024

2) Breaking long lines and strings

DimitriPapadopoulos commented Jun 17, 2024

abravalheri commented Jun 17, 2024 • edited Loading

abravalheri commented Jun 17, 2024

DimitriPapadopoulos commented Jun 17, 2024

DimitriPapadopoulos commented Jun 17, 2024

abravalheri Jun 17, 2024 •

edited

Loading

abravalheri Jun 17, 2024 •

edited

Loading

abravalheri commented Jun 17, 2024 •

edited

Loading