Improve IPython escape command parsing logic by dhruvmanila · Pull Request #10873 · astral-sh/ruff

dhruvmanila · 2024-04-11T09:39:17Z

Summary

This PR reviews the logic for IPython escape command parsing and makes the required changes to almost match the old parsing logic.

~~One change I've not incorporated is the unparse_expr. The new parser can use src_text directly but this means that any whitespace is preserved~~ Migrated the unparse_expr logic

Test plan

Use the new parser for testing escape commands and verify that there are no snapshot changes.

MichaReiser · 2024-04-11T09:40:53Z

This PR reviews the logic for IPython escape command parsing and makes the required changes to almost match the old parsing logic.

What does that mean. Where does it not match the old parsing logic and why is that?

dhruvmanila · 2024-04-11T09:40:29Z

crates/ruff_python_parser/src/parser/statement.rs

            TokenKind::Type => Stmt::TypeAlias(self.parse_type_alias_statement()),
-            TokenKind::EscapeCommand if self.mode == Mode::Ipython => {
+            TokenKind::IpyEscapeCommand => {
                Stmt::IpyEscapeCommand(self.parse_ipython_escape_command_statement())
            }


I don't think we should let it use the fallback logic and instead handle it in parse_ipython_escape_command_statement function.

dhruvmanila · 2024-04-11T09:42:19Z

crates/ruff_python_parser/src/parser/statement.rs

+        } else if !matches!(
+            parsed_expr.expr,
+            Expr::Name(_) | Expr::Attribute(_) | Expr::Subscript(_)
+        ) {
+            self.add_error(
+                ParseErrorType::OtherError(
+                    "Expected name, subscript or attribute expression in help end escape command"
+                        .to_string(),
+                ),
+                parsed_expr,
+            );
+        }


I haven't migrated the unparse_expr used in the old parser.

@MichaReiser this is where it doesn't match.

Nevermind, it wasn't too much work migrating it.

codspeed-hq · 2024-04-11T10:20:53Z

CodSpeed Performance Report

Merging #10873 will degrade performances by 5.25%

_{Comparing dhruv/ipy-escape-command (90fd01e) with dhruv/parser (6b4e771)}

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 27 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`dhruv/parser`	`dhruv/ipy-escape-command`	Change
⚡	`parser[large/dataset.py]`	28.9 ms	26.7 ms	+8.24%
⚡	`parser[pydantic/types.py]`	11.8 ms	10.9 ms	+8.78%
❌	`parser[unicode/pypinyin.py]`	1.6 ms	1.7 ms	-5.25%

github-actions · 2024-04-11T10:35:00Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+7 -7 violations, +0 -0 fixes in 3 projects; 41 projects unchanged)

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

+ airflow/migrations/env.py:33:5: SIM103 Return the condition directly
- airflow/migrations/env.py:33:5: SIM103 Return the negated condition directly
+ scripts/ci/pre_commit/check_integrations_list.py:113:9: SIM103 Return the condition `j in ["Description", "Identifier"]` directly
- scripts/ci/pre_commit/check_integrations_list.py:113:9: SIM103 Return the condition `not j in ["Description", "Identifier"]` directly

demisto/content (+4 -4 violations, +0 -0 fixes)

- Packs/CimTrak-SystemIntegrityAssurance/Integrations/CimTrak/CimTrak.py:1606:5: SIM103 Return the condition `not (value.lower() == "false" or value == "0")` directly
+ Packs/CimTrak-SystemIntegrityAssurance/Integrations/CimTrak/CimTrak.py:1606:5: SIM103 Return the condition `value.lower() == "false" or value == "0"` directly
+ Packs/ContentManagement/Scripts/ConfigurationSetup/ConfigurationSetup.py:221:5: SIM103 Return the condition directly
- Packs/ContentManagement/Scripts/ConfigurationSetup/ConfigurationSetup.py:221:5: SIM103 Return the negated condition directly
+ Packs/Malwarebytes/Integrations/Malwarebytes/Malwarebytes.py:1432:13: SIM103 Return the condition `data.get("statusCode") == 201` directly
- Packs/Malwarebytes/Integrations/Malwarebytes/Malwarebytes.py:1432:13: SIM103 Return the condition `data.get('statusCode') == 201` directly
+ Packs/TrendMicroDDA/Integrations/TrendMicroDDA/TrendMicroDDA.py:126:5: SIM103 Return the condition `binary == '0'` directly
- Packs/TrendMicroDDA/Integrations/TrendMicroDDA/TrendMicroDDA.py:126:5: SIM103 Return the condition `not binary == "0"` directly

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

- zproject/backends.py:628:5: SIM103 Return the condition `bool(find_ldap_users_by_email(email))` directly
+ zproject/backends.py:628:5: SIM103 Return the condition `find_ldap_users_by_email(email)` directly

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
SIM103	14	7	7	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+97 -237 violations, +0 -0 fixes in 12 projects; 32 projects unchanged)

apache/airflow (+24 -62 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- airflow/cli/commands/kubernetes_command.py:70:14: FURB103 `open` and `write` should be replaced by `Path(...).write_text(yaml.dump(sanitized_pod))`
- airflow/cli/commands/pool_command.py:147:10: FURB103 `open` and `write` should be replaced by `Path(filepath)....`
- airflow/configuration.py:1309:13: SIM103 Return the condition `not value is None` directly
+ airflow/configuration.py:1309:13: SIM103 Return the condition `value is None` directly
- airflow/dag_processing/manager.py:1248:9: SIM103 Return the condition `not self._num_run < self._max_runs` directly
+ airflow/dag_processing/manager.py:1248:9: SIM103 Return the condition `self._num_run < self._max_runs` directly
- airflow/example_dags/example_kubernetes_executor.py:91:18: FURB103 `open` and `write` should be replaced by `Path("/foo/volume_mount_test.txt").write_text("Hello")`
+ airflow/migrations/env.py:33:5: SIM103 Return the condition directly
- airflow/migrations/env.py:33:5: SIM103 Return the negated condition directly
+ airflow/models/taskinstance.py:385:5: SIM103 Return the condition `isinstance(value, (bytearray, bytes, str))` directly
... 42 additional changes omitted for rule SIM103
- airflow/providers/databricks/operators/databricks_sql.py:152:18: FURB103 `open` and `write` should be replaced by `Path(self._output_path)....`
- airflow/providers/fab/auth_manager/security_manager/override.py:776:18: FURB103 `open` and `write` should be replaced by `Path(password_path).write_text(password)`
... 74 additional changes omitted for project

bokeh/bokeh (+1 -42 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- examples/models/basic_plot.py:42:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
- examples/models/buttons.py:75:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Button widgets", doc))`
- examples/models/calendars.py:104:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Calendar 2014", doc))`
- examples/models/colors.py:69:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
- examples/models/custom.py:117:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
- examples/models/customjs.py:38:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
... 36 additional changes omitted for rule FURB103
- examples/server/app/server_auth/auth.py:40:9: SIM103 Return the condition `bool(username == "bokeh" and password == "bokeh")` directly
+ examples/server/app/server_auth/auth.py:40:9: SIM103 Return the condition `username == "bokeh" and password == "bokeh"` directly
... 35 additional changes omitted for project

demisto/content (+25 -25 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:763:9: SIM103 Return the condition `RDL is None` directly
- Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:763:9: SIM103 Return the negated condition directly
- Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:778:9: SIM103 Return the condition `not not self._valid(self.rcs)` directly
+ Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:778:9: SIM103 Return the condition `not self._valid(self.rcs)` directly
- Packs/Active_Directory_Query/Integrations/Active_Directory_Query/Active_Directory_Query.py:347:5: SIM103 Return the condition `bool(entries.get('flat'))` directly
+ Packs/Active_Directory_Query/Integrations/Active_Directory_Query/Active_Directory_Query.py:347:5: SIM103 Return the condition `entries.get('flat')` directly
+ Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py:540:5: SIM103 Return the condition `not 1 < n_labels < n_samples` directly
- Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py:540:5: SIM103 Return the condition `not not 1 < n_labels < n_samples` directly
- Packs/CheckPhish/Integrations/CheckPhish/CheckPhish.py:144:5: SIM103 Return the condition `bool(res and res['status'] == DONE_STATUS)` directly
+ Packs/CheckPhish/Integrations/CheckPhish/CheckPhish.py:144:5: SIM103 Return the condition `res and res['status'] == DONE_STATUS` directly
... 40 additional changes omitted for project

freedomofpress/securedrop (+6 -6 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- journalist_gui/journalist_gui/SecureDropUpdater.py:23:5: SIM103 Return the condition `not pwd_flag == "NP"` directly
+ journalist_gui/journalist_gui/SecureDropUpdater.py:23:5: SIM103 Return the condition `pwd_flag == "NP"` directly
- securedrop/pretty_bad_protocol/_parsers.py:1008:9: SIM103 Return the condition `bool(self.fingerprint)` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1008:9: SIM103 Return the condition `self.fingerprint` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1321:9: SIM103 Return the condition `len(self.fingerprints) == 0` directly
- securedrop/pretty_bad_protocol/_parsers.py:1321:9: SIM103 Return the condition `not len(self.fingerprints) == 0` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1425:9: SIM103 Return the condition `len(self.fingerprints) == 0` directly
- securedrop/pretty_bad_protocol/_parsers.py:1425:9: SIM103 Return the condition `not len(self.fingerprints) == 0` directly
- securedrop/pretty_bad_protocol/_parsers.py:1788:9: SIM103 Return the condition `bool(self.ok)` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1788:9: SIM103 Return the condition `self.ok` directly
... 2 additional changes omitted for project

milvus-io/pymilvus (+5 -5 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pymilvus/client/check.py:166:5: SIM103 Return the condition `(end_date - start_date).days < 0` directly
- pymilvus/client/check.py:166:5: SIM103 Return the condition `not (end_date - start_date).days < 0` directly
+ pymilvus/client/check.py:20:5: SIM103 Return the condition `not is_legal_host(a[0]) or not is_legal_port(a[1])` directly
- pymilvus/client/check.py:20:5: SIM103 Return the negated condition directly
+ pymilvus/client/check.py:27:5: SIM103 Return the condition directly
- pymilvus/client/check.py:27:5: SIM103 Return the negated condition directly
+ pymilvus/orm/collection.py:1445:9: SIM103 Return the condition directly
- pymilvus/orm/collection.py:1445:9: SIM103 Return the negated condition directly
+ pymilvus/orm/iterator.py:458:9: SIM103 Return the condition `cached_page is None or len(cached_page) < count` directly
- pymilvus/orm/iterator.py:458:9: SIM103 Return the negated condition directly

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ mlflow/sklearn/utils.py:929:9: SIM103 Return the condition `not len(c.__abstractmethods__)` directly
- mlflow/sklearn/utils.py:929:9: SIM103 Return the condition `not not len(c.__abstractmethods__)` directly
+ mlflow/tracking/_tracking_service/utils.py:25:5: SIM103 Return the condition `_tracking_uri or MLFLOW_TRACKING_URI.get()` directly
- mlflow/tracking/_tracking_service/utils.py:25:5: SIM103 Return the condition `bool(_tracking_uri or MLFLOW_TRACKING_URI.get())` directly
+ mlflow/transformers/__init__.py:1473:5: SIM103 Return the condition directly
- mlflow/transformers/__init__.py:1473:5: SIM103 Return the negated condition directly
+ mlflow/utils/logging_utils.py:101:9: SIM103 Return the condition directly
- mlflow/utils/logging_utils.py:101:9: SIM103 Return the negated condition directly
+ mlflow/utils/search_utils.py:1199:9: SIM103 Return the condition directly
- mlflow/utils/search_utils.py:1199:9: SIM103 Return the negated condition directly
... 8 additional changes omitted for project

pypa/cibuildwheel (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ cibuildwheel/projectfiles.py:42:5: SIM103 Return the condition `len(consts) != 1` directly
- cibuildwheel/projectfiles.py:42:5: SIM103 Return the condition `not len(consts) != 1` directly

reflex-dev/reflex (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- reflex/utils/telemetry.py:85:5: SIM103 Return the condition `not should_skip_compile()` directly
+ reflex/utils/telemetry.py:85:5: SIM103 Return the condition `should_skip_compile()` directly

rotki/rotki (+0 -19 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- package.py:734:14: FURB103 `open` and `write` should be replaced by `Path(p12).write_bytes(certificate_data)`
- rotkehlchen/chain/ethereum/airdrops.py:248:18: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(response.text, encoding='utf8')`
- rotkehlchen/chain/ethereum/airdrops.py:284:14: FURB103 `open` and `write` should be replaced by `Path(filename)....`
- rotkehlchen/chain/ethereum/utils.py:224:10: FURB103 `open` and `write` should be replaced by `Path(avatars_dir / f'{ens_name}.png').write_bytes(avatar)`
- rotkehlchen/db/dbhandler.py:300:14: FURB103 `open` and `write` should be replaced by `Path(self.user_data_dir / DBINFO_FILENAME).write_text(rlk_jsondumps(dbinfo), encoding='utf8')`
- rotkehlchen/db/dbhandler.py:593:18: FURB103 `open` and `write` should be replaced by `Path(tempdbpath).write_bytes(unencrypted_db_data)`
- rotkehlchen/icons.py:190:14: FURB103 `open` and `write` should be replaced by `Path(self.iconfile_path(asset)).write_bytes(response.content)`
- rotkehlchen/tests/api/test_caching.py:42:10: FURB103 `open` and `write` should be replaced by `Path(f'{icons_dir}/ETH_small.png').write_bytes(b'')`
- rotkehlchen/tests/api/test_caching.py:45:10: FURB103 `open` and `write` should be replaced by `Path(f'{icons_dir}/BTC_small.png').write_bytes(b'')`
- rotkehlchen/tests/api/test_caching.py:48:10: FURB103 `open` and `write` should be replaced by `Path(f'{icons_dir}/AVAX_small.png').write_bytes(b'')`
... 9 additional changes omitted for project

... Truncated remaining completed project reports due to GitHub comment length restrictions

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
SIM103	194	97	97	0	0
FURB103	140	0	140	0	0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

## Summary This PR reviews the logic for IPython escape command parsing and makes the required changes to almost match the old parsing logic. ~One change I've not incorporated is the `unparse_expr`. The new parser can use `src_text` directly but this means that any whitespace is preserved~ Migrated the `unparse_expr` logic ## Test plan Use the new parser for testing escape commands and verify that there are no snapshot changes.

dhruvmanila added the parser Related to the parser label Apr 11, 2024

dhruvmanila requested a review from MichaReiser as a code owner April 11, 2024 09:39

dhruvmanila commented Apr 11, 2024

View reviewed changes

dhruvmanila force-pushed the dhruv/review-error-message branch from 9b49864 to 668b07d Compare April 11, 2024 09:53

dhruvmanila force-pushed the dhruv/ipy-escape-command branch from 62a9cfc to 04337ac Compare April 11, 2024 10:13

dhruvmanila force-pushed the dhruv/review-error-message branch from 668b07d to ea0b7eb Compare April 11, 2024 10:14

Base automatically changed from dhruv/review-error-message to dhruv/parser April 11, 2024 10:15

Improve IPython escape command parsing logic

90fd01e

dhruvmanila force-pushed the dhruv/ipy-escape-command branch from 04337ac to 90fd01e Compare April 11, 2024 10:15

dhruvmanila merged commit 0f008cc into dhruv/parser Apr 11, 2024

dhruvmanila deleted the dhruv/ipy-escape-command branch April 11, 2024 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve IPython escape command parsing logic#10873

Improve IPython escape command parsing logic#10873
dhruvmanila merged 1 commit intodhruv/parserfrom
dhruv/ipy-escape-command

dhruvmanila commented Apr 11, 2024 •

edited

Loading

Uh oh!

MichaReiser commented Apr 11, 2024

Uh oh!

dhruvmanila Apr 11, 2024

Uh oh!

dhruvmanila Apr 11, 2024

Uh oh!

dhruvmanila Apr 11, 2024

Uh oh!

dhruvmanila Apr 11, 2024

Uh oh!

codspeed-hq bot commented Apr 11, 2024

Uh oh!

github-actions bot commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dhruvmanila commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

MichaReiser commented Apr 11, 2024

Uh oh!

dhruvmanila Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Apr 11, 2024

CodSpeed Performance Report

Merging #10873 will degrade performances by 5.25%

Summary

Benchmarks breakdown

Uh oh!

github-actions bot commented Apr 11, 2024

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dhruvmanila commented Apr 11, 2024 •

edited

Loading

`ruff-ecosystem` results