Skip to content

Comments

Add flake8-implicit-str-concat check to Ruff#36597

Merged
josh-fell merged 1 commit intoapache:mainfrom
josh-fell:ruff-isc
Jan 5, 2024
Merged

Add flake8-implicit-str-concat check to Ruff#36597
josh-fell merged 1 commit intoapache:mainfrom
josh-fell:ruff-isc

Conversation

@josh-fell
Copy link
Contributor

@josh-fell josh-fell commented Jan 4, 2024 •
edited
Loading

This check was enabled initially in #23873, but hasn't been part of the Ruff checks to-date. Let's add it!

o-nikolas
o-nikolas reviewed Jan 4, 2024
View reviewed changes
@o-nikolas
Copy link
Contributor

o-nikolas commented Jan 4, 2024

Other than one comment, looks good to me! Love this one ❤️

@josh-fell josh-fell requested a review from o-nikolas January 4, 2024 18:36
@potiuk
Copy link
Member

potiuk commented Jan 4, 2024

Other than one comment, looks good to me! Love this one ❤️

Likewise :)

pankajkoti
pankajkoti approved these changes Jan 4, 2024
View reviewed changes
Copy link
Member

@pankajkoti pankajkoti left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, just wondering how those string concatenations existed in so many places :) maybe it was another pre-commit responsible for it.

@potiuk
Copy link
Member

potiuk commented Jan 4, 2024 •
edited
Loading

nice, just wondering how those string concatenations existed in so many places :) maybe it was another pre-commit responsible for it.

We used to get rid of them by flake I think - but at the time we moved to ruff it was not available as far as I remember.
And those strings are created by black reformatting - for example when you remove if and the whole block gets de-dented - the previously split strings (because they were too long) are moved to one line. But black (and ruff-black follows it) has the rule that it never EVER changes the AST so concatenating two strings is not possible in ruff.

That's the root cause we had so many of those.

And yes. I got far too many of those as well and it started to be annoying :)

@josh-fell
Add flake8-implicit-str-concat check to Ruff
e996cc3 
This was enabled initially in apache#23873, but hasn't been part of the Ruff checks to-date. Let's add it!
@josh-fell josh-fell merged commit 1cc9fe1 into apache:main Jan 5, 2024
@josh-fell josh-fell deleted the ruff-isc branch January 5, 2024 14:22
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 10, 2024
@ephraimbuddy ephraimbuddy added this to the Airflow 2.9.0 milestone Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@Taragolis Taragolis Taragolis approved these changes

@pankajkoti pankajkoti pankajkoti approved these changes

@hussein-awala hussein-awala hussein-awala approved these changes

@eladkal eladkal eladkal approved these changes

@o-nikolas o-nikolas o-nikolas approved these changes

@jscheffl jscheffl jscheffl approved these changes

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

Assignees

No one assigned

Labels

area:API Airflow's REST/HTTP API area:dev-tools area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:amazon AWS/Amazon - related issues provider:weaviate

Projects

None yet

Milestone

Airflow 2.9.0

Development

Successfully merging this pull request may close these issues.

9 participants

@josh-fell @o-nikolas @potiuk @Taragolis @pankajkoti @hussein-awala @eladkal @jscheffl @ephraimbuddy