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

[flake8-pie] Omit bound tuples passed to .startswith or .endswith #9661

Conversation

mikeleppane
Copy link
Contributor

@mikeleppane mikeleppane commented Jan 28, 2024

Summary

This review contains a fix for PIE810 (multiple-starts-ends-with)

The problem is that ruff suggests combining multiple startswith/endswith calls into a single call even though there might be a call with tuple of strs. This leads to calling startswith/endswith with tuple of tuple of strs which is incorrect and violates startswith/endswith conctract and results in runtime failure.

However the following will be valid and fixed correctly =>

x = ("hello", "world")
y = "h"
z = "w"
msg = "hello world"

if msg.startswith(x) or msg.startswith(y) or msg.startswith(z) :
      sys.exit(1)
ruff --fix --select PIE810 --unsafe-fixes

=>

if msg.startswith(x) or msg.startswith((y,z)):
      sys.exit(1)

See: #8906

Test Plan

cargo test

@mikeleppane mikeleppane changed the title [pie-fix]: Incorrect suggestion when calling startswith or endswith with tuple of strs [flake8pie-fix]: Incorrect suggestion when calling startswith or endswith with tuple of strs Jan 28, 2024
Copy link
Contributor

github-actions bot commented Jan 28, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+17 -20 violations, +0 -0 fixes in 4 projects; 39 projects unchanged)

apache/airflow (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

+ airflow/providers/google/suite/hooks/drive.py:197:13: SIM108 Use ternary operator `path = f"{file_info['name']}" if current_file_id == file_id else f"{file_info['name']}/{path}"` instead of `if`-`else`-block
- airflow/providers/google/suite/hooks/drive.py:197:13: SIM108 Use ternary operator `path = f'{file_info["name"]}' if current_file_id == file_id else f'{file_info["name"]}/{path}'` instead of `if`-`else`-block

bokeh/bokeh (+14 -14 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

+ examples/reference/models/select_server.py:28:5: SIM108 Use ternary operator `selected_df = df[df['label'] == active_select] if active_select != 'All' else df.copy()` instead of `if`-`else`-block
- examples/reference/models/select_server.py:28:5: SIM108 Use ternary operator `selected_df = df[df['label']==active_select] if active_select!='All' else df.copy()` instead of `if`-`else`-block
- examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- src/bokeh/command/subcommands/file_output.py:134:9: SIM108 Use ternary operator `base = "index" if route == "/" else route[1:]` instead of `if`-`else`-block
+ src/bokeh/command/subcommands/file_output.py:134:9: SIM108 Use ternary operator `base = 'index' if route == '/' else route[1:]` instead of `if`-`else`-block
- src/bokeh/core/json_encoder.py:153:5: SIM108 Use ternary operator `separators = (",", ": ") if pretty else (",", ":")` instead of `if`-`else`-block
+ src/bokeh/core/json_encoder.py:153:5: SIM108 Use ternary operator `separators = (',', ': ') if pretty else (',', ':')` instead of `if`-`else`-block
- src/bokeh/plotting/_graph.py:105:5: SIM108 Use ternary operator `sedge_visuals = pop_visuals(MultiLine, kwargs, prefix="edge_selection_", defaults=edge_visuals) if any(x.startswith('edge_selection_') for x in kwargs) else None` instead of `if`-`else`-block
+ src/bokeh/plotting/_graph.py:105:5: SIM108 Use ternary operator `sedge_visuals = pop_visuals(MultiLine, kwargs, prefix='edge_selection_', defaults=edge_visuals) if any(x.startswith('edge_selection_') for x in kwargs) else None` instead of `if`-`else`-block
- src/bokeh/plotting/_graph.py:110:5: SIM108 Use ternary operator `hedge_visuals = pop_visuals(MultiLine, kwargs, prefix="edge_hover_", defaults=edge_visuals) if any(x.startswith('edge_hover_') for x in kwargs) else None` instead of `if`-`else`-block
+ src/bokeh/plotting/_graph.py:110:5: SIM108 Use ternary operator `hedge_visuals = pop_visuals(MultiLine, kwargs, prefix='edge_hover_', defaults=edge_visuals) if any(x.startswith('edge_hover_') for x in kwargs) else None` instead of `if`-`else`-block
- src/bokeh/plotting/_graph.py:93:5: SIM108 Use ternary operator `hnode_visuals = pop_visuals(marker_type, kwargs, prefix="node_hover_", defaults=node_visuals) if any(x.startswith('node_hover_') for x in kwargs) else None` instead of `if`-`else`-block
+ src/bokeh/plotting/_graph.py:93:5: SIM108 Use ternary operator `hnode_visuals = pop_visuals(marker_type, kwargs, prefix='node_hover_', defaults=node_visuals) if any(x.startswith('node_hover_') for x in kwargs) else None` instead of `if`-`else`-block
- src/bokeh/server/tornado.py:290:9: SIM108 Use ternary operator `applications = {'/' : applications} if isinstance(applications, Application) else dict(applications)` instead of `if`-`else`-block
+ src/bokeh/server/tornado.py:290:9: SIM108 Use ternary operator `applications = {'/': applications} if isinstance(applications, Application) else dict(applications)` instead of `if`-`else`-block
- src/bokeh/server/tornado.py:414:17: SIM108 Use ternary operator `route = p[0] if key == "/" else key + p[0]` instead of `if`-`else`-block
+ src/bokeh/server/tornado.py:414:17: SIM108 Use ternary operator `route = p[0] if key == '/' else key + p[0]` instead of `if`-`else`-block
- src/bokeh/util/compiler.py:253:13: SIM108 Use ternary operator `impl = FromFile(impl if isabs(impl) else join(self.path, impl)) if "\n" not in impl and impl.endswith(exts) else TypeScript(impl)` instead of `if`-`else`-block
+ src/bokeh/util/compiler.py:253:13: SIM108 Use ternary operator `impl = FromFile(impl if isabs(impl) else join(self.path, impl)) if '\n' not in impl and impl.endswith(exts) else TypeScript(impl)` instead of `if`-`else`-block
- tests/support/util/examples.py:217:9: SIM108 Use ternary operator `example_type = getattr(Flags, example["type"]) if example.get("type") is not None else None` instead of `if`-`else`-block
+ tests/support/util/examples.py:217:9: SIM108 Use ternary operator `example_type = getattr(Flags, example['type']) if example.get('type') is not None else None` instead of `if`-`else`-block

mlflow/mlflow (+0 -1 violations, +0 -0 fixes)

- tests/pyfunc/test_pyfunc_schema_enforcement.py:2138:5: SIM108 Use ternary operator `df = pd.DataFrame([data]) if isinstance(data, dict) and all(

zulip/zulip (+2 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

- analytics/tests/test_activity_views.py:236:17: SIM108 Use ternary operator `price_per_license = 0 if tier in (
- scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if "." in shard else f"{shard}.{external_host}"` instead of `if`-`else`-block
+ scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if '.' in shard else f'{shard}.{external_host}'` instead of `if`-`else`-block
- zerver/migrations/0182_set_initial_value_is_private_flag.py:37:9: SIM108 Use ternary operator `percent = round((processed / total) * 100, 2) if total != 0 else 100.00` instead of `if`-`else`-block
+ zerver/migrations/0182_set_initial_value_is_private_flag.py:37:9: SIM108 Use ternary operator `percent = round(processed / total * 100, 2) if total != 0 else 100.0` instead of `if`-`else`-block
- zerver/tests/test_settings.py:371:9: SIM108 Use ternary operator `data = {setting_name: test_value} if setting_name not in [

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM108 37 17 20 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+17 -20 violations, +0 -0 fixes in 4 projects; 39 projects unchanged)

apache/airflow (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ airflow/providers/google/suite/hooks/drive.py:197:13: SIM108 Use ternary operator `path = f"{file_info['name']}" if current_file_id == file_id else f"{file_info['name']}/{path}"` instead of `if`-`else`-block
- airflow/providers/google/suite/hooks/drive.py:197:13: SIM108 Use ternary operator `path = f'{file_info["name"]}' if current_file_id == file_id else f'{file_info["name"]}/{path}'` instead of `if`-`else`-block

bokeh/bokeh (+14 -14 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ examples/reference/models/select_server.py:28:5: SIM108 Use ternary operator `selected_df = df[df['label'] == active_select] if active_select != 'All' else df.copy()` instead of `if`-`else`-block
- examples/reference/models/select_server.py:28:5: SIM108 Use ternary operator `selected_df = df[df['label']==active_select] if active_select!='All' else df.copy()` instead of `if`-`else`-block
- examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/flask_embed.py:26:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/flask_gunicorn_embed.py:41:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/standalone_embed.py:18:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f"{new}D").mean()` instead of `if`-`else`-block
+ examples/server/api/tornado_embed.py:29:9: SIM108 Use ternary operator `data = df if new == 0 else df.rolling(f'{new}D').mean()` instead of `if`-`else`-block
- src/bokeh/command/subcommands/file_output.py:134:9: SIM108 Use ternary operator `base = "index" if route == "/" else route[1:]` instead of `if`-`else`-block
+ src/bokeh/command/subcommands/file_output.py:134:9: SIM108 Use ternary operator `base = 'index' if route == '/' else route[1:]` instead of `if`-`else`-block
- src/bokeh/core/json_encoder.py:153:5: SIM108 Use ternary operator `separators = (",", ": ") if pretty else (",", ":")` instead of `if`-`else`-block
+ src/bokeh/core/json_encoder.py:153:5: SIM108 Use ternary operator `separators = (',', ': ') if pretty else (',', ':')` instead of `if`-`else`-block
- src/bokeh/plotting/_graph.py:105:5: SIM108 Use ternary operator `sedge_visuals = pop_visuals(MultiLine, kwargs, prefix="edge_selection_", defaults=edge_visuals) if any(x.startswith('edge_selection_') for x in kwargs) else None` instead of `if`-`else`-block
+ src/bokeh/plotting/_graph.py:105:5: SIM108 Use ternary operator `sedge_visuals = pop_visuals(MultiLine, kwargs, prefix='edge_selection_', defaults=edge_visuals) if any(x.startswith('edge_selection_') for x in kwargs) else None` instead of `if`-`else`-block
- src/bokeh/plotting/_graph.py:110:5: SIM108 Use ternary operator `hedge_visuals = pop_visuals(MultiLine, kwargs, prefix="edge_hover_", defaults=edge_visuals) if any(x.startswith('edge_hover_') for x in kwargs) else None` instead of `if`-`else`-block
+ src/bokeh/plotting/_graph.py:110:5: SIM108 Use ternary operator `hedge_visuals = pop_visuals(MultiLine, kwargs, prefix='edge_hover_', defaults=edge_visuals) if any(x.startswith('edge_hover_') for x in kwargs) else None` instead of `if`-`else`-block
- src/bokeh/plotting/_graph.py:93:5: SIM108 Use ternary operator `hnode_visuals = pop_visuals(marker_type, kwargs, prefix="node_hover_", defaults=node_visuals) if any(x.startswith('node_hover_') for x in kwargs) else None` instead of `if`-`else`-block
+ src/bokeh/plotting/_graph.py:93:5: SIM108 Use ternary operator `hnode_visuals = pop_visuals(marker_type, kwargs, prefix='node_hover_', defaults=node_visuals) if any(x.startswith('node_hover_') for x in kwargs) else None` instead of `if`-`else`-block
- src/bokeh/server/tornado.py:290:9: SIM108 Use ternary operator `applications = {'/' : applications} if isinstance(applications, Application) else dict(applications)` instead of `if`-`else`-block
+ src/bokeh/server/tornado.py:290:9: SIM108 Use ternary operator `applications = {'/': applications} if isinstance(applications, Application) else dict(applications)` instead of `if`-`else`-block
- src/bokeh/server/tornado.py:414:17: SIM108 Use ternary operator `route = p[0] if key == "/" else key + p[0]` instead of `if`-`else`-block
+ src/bokeh/server/tornado.py:414:17: SIM108 Use ternary operator `route = p[0] if key == '/' else key + p[0]` instead of `if`-`else`-block
- src/bokeh/util/compiler.py:253:13: SIM108 Use ternary operator `impl = FromFile(impl if isabs(impl) else join(self.path, impl)) if "\n" not in impl and impl.endswith(exts) else TypeScript(impl)` instead of `if`-`else`-block
+ src/bokeh/util/compiler.py:253:13: SIM108 Use ternary operator `impl = FromFile(impl if isabs(impl) else join(self.path, impl)) if '\n' not in impl and impl.endswith(exts) else TypeScript(impl)` instead of `if`-`else`-block
- tests/support/util/examples.py:217:9: SIM108 Use ternary operator `example_type = getattr(Flags, example["type"]) if example.get("type") is not None else None` instead of `if`-`else`-block
+ tests/support/util/examples.py:217:9: SIM108 Use ternary operator `example_type = getattr(Flags, example['type']) if example.get('type') is not None else None` instead of `if`-`else`-block

mlflow/mlflow (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

- tests/pyfunc/test_pyfunc_schema_enforcement.py:2138:5: SIM108 Use ternary operator `df = pd.DataFrame([data]) if isinstance(data, dict) and all(

zulip/zulip (+2 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- analytics/tests/test_activity_views.py:236:17: SIM108 Use ternary operator `price_per_license = 0 if tier in (
- scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if "." in shard else f"{shard}.{external_host}"` instead of `if`-`else`-block
+ scripts/lib/sharding.py:65:21: SIM108 Use ternary operator `host = shard if '.' in shard else f'{shard}.{external_host}'` instead of `if`-`else`-block
- zerver/migrations/0182_set_initial_value_is_private_flag.py:37:9: SIM108 Use ternary operator `percent = round((processed / total) * 100, 2) if total != 0 else 100.00` instead of `if`-`else`-block
+ zerver/migrations/0182_set_initial_value_is_private_flag.py:37:9: SIM108 Use ternary operator `percent = round(processed / total * 100, 2) if total != 0 else 100.0` instead of `if`-`else`-block
- zerver/tests/test_settings.py:371:9: SIM108 Use ternary operator `data = {setting_name: test_value} if setting_name not in [

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM108 37 17 20 0 0

@charliermarsh charliermarsh changed the title [flake8pie-fix]: Incorrect suggestion when calling startswith or endswith with tuple of strs [flake8-pie] Destructure tuples when passed to .startswith or .endswith Jan 28, 2024
@charliermarsh charliermarsh added bug Something isn't working fixes Related to suggested fixes for violations labels Jan 28, 2024
@charliermarsh charliermarsh force-pushed the fix(PIE810)/wrong_suggestion_with_tuple_of_strs branch from 15307e4 to d02dc26 Compare January 28, 2024 19:23
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks!

@charliermarsh charliermarsh enabled auto-merge (squash) January 28, 2024 19:24
@charliermarsh charliermarsh changed the title [flake8-pie] Destructure tuples when passed to .startswith or .endswith [flake8-pie] Omit bound tuples passed to .startswith or .endswith Jan 28, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) January 28, 2024 19:24
@charliermarsh charliermarsh merged commit b9139a3 into astral-sh:main Jan 28, 2024
16 checks passed
@deliro
Copy link

deliro commented Jan 28, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants