Skip to content

Conversation

@abhijeetbodas2001
Copy link
Contributor

@abhijeetbodas2001 abhijeetbodas2001 commented Apr 22, 2025

Summary

Part of #17412

Add a new compile-time syntax error for detecting nonlocal declarations at a module level.

Test Plan

  • Added new inline tests for the syntax error
  • Updated existing tests for nonlocal statement parsing to be inside a function scope

@abhijeetbodas2001
Copy link
Contributor Author

@dhruvmanila could you review this?

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I had one suggestion about where to put tests, but this looks great to me.

@dhruvmanila dhruvmanila removed their request for review April 23, 2025 15:14
@dhruvmanila
Copy link
Member

(I'll leave this to Brent)

@abhijeetbodas2001
Copy link
Contributor Author

abhijeetbodas2001 commented Apr 23, 2025

@ntBre Thanks for the review. I've addressed your comment and pushed again.

I have one question -- since it looks like this repo follows the squash+merge git workflow, should I be updating the PR description every time I push after addressing review comments (since this description will end up in the commit message)?

(Edit: updated the PR description)

@abhijeetbodas2001 abhijeetbodas2001 requested a review from ntBre April 24, 2025 02:38
@abhijeetbodas2001
Copy link
Contributor Author

(Resolved conflicts in the last push)

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@ntBre
Copy link
Contributor

ntBre commented Apr 24, 2025

I have one question -- since it looks like this repo follows the squash+merge git workflow, should I be updating the PR description every time I push after addressing review comments (since this description will end up in the commit message)?

Great question! I would say not to worry too much about it, but it is definitely nice to keep it updated if things change substantially. Your edits here look perfect.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

- providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:353:36: AIR311 [*] `airflow.datasets.DatasetAny` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
+ providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:353:36: AIR311 `airflow.datasets.DatasetAny` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
- providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:353:48: AIR311 [*] `airflow.datasets.DatasetAll` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
+ providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:353:48: AIR311 `airflow.datasets.DatasetAll` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
- providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:355:67: AIR311 [*] `airflow.datasets.DatasetAll` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
+ providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:355:67: AIR311 `airflow.datasets.DatasetAll` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
- providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:678:18: AIR311 [*] `airflow.timetables.datasets.DatasetOrTimeSchedule` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
+ providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:678:18: AIR311 `airflow.timetables.datasets.DatasetOrTimeSchedule` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
- providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:774:18: AIR311 [*] `airflow.timetables.datasets.DatasetOrTimeSchedule` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
+ providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:774:18: AIR311 `airflow.timetables.datasets.DatasetOrTimeSchedule` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
- providers/openlineage/tests/unit/openlineage/utils/test_utils.py:985:22: AIR311 [*] `airflow.timetables.datasets.DatasetOrTimeSchedule` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.
+ providers/openlineage/tests/unit/openlineage/utils/test_utils.py:985:22: AIR311 `airflow.timetables.datasets.DatasetOrTimeSchedule` is removed in Airflow 3.0; It still works in Airflow 3.0 but is expected to be removed in a future version.

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
AIR311 12 0 0 0 12

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@ntBre ntBre changed the title [syntax-errors] nonlocal declaration at module level [syntax-errors] nonlocal declaration at module level Apr 24, 2025
@ntBre ntBre added rule Implementing or modifying a lint rule preview Related to preview mode features labels Apr 24, 2025
@ntBre ntBre merged commit cf59cee into astral-sh:main Apr 24, 2025
33 checks passed
@abhijeetbodas2001 abhijeetbodas2001 deleted the apb branch April 25, 2025 03:48
@abhijeetbodas2001
Copy link
Contributor Author

Great question! I would say not to worry too much about it, but it is definitely nice to keep it updated if things change substantially. Your edits here look perfect.

Got it, thanks!

Also wanted to mention that your detailed issue write up was really helpful from a new contributor (ie, me) POV, so thanks for that as well 😄

@ntBre
Copy link
Contributor

ntBre commented Apr 25, 2025

Thank you, that's great to hear! And thanks for the great PRs!

dcreager added a commit that referenced this pull request Apr 25, 2025
* main: (24 commits)
  [`airflow`] Extend `AIR301` rule (#17598)
  [`airflow`] update existing `AIR302` rules with better suggestions (#17542)
  red_knot_project: sort diagnostics from checking files
  [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621)
  [red-knot] fix inheritance-cycle detection for generic classes (#17620)
  [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411)
  Add Semantic Error Test for LateFutureImport (#17612)
  [red-knot] change TypeVarInstance to be interned, not tracked (#17616)
  [red-knot] Special case `@final`, `@override` (#17608)
  [red-knot] add TODO comment in specialization code (#17615)
  [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592)
  [syntax-errors] `nonlocal` declaration at module level (#17559)
  [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571)
  [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460)
  Bump 0.11.7 (#17613)
  [red-knot] Use iterative approach to collect overloads (#17607)
  red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest
  red_knot_python_semantic: improve diagnostics for unsupported boolean conversions
  red_knot_python_semantic: add "return type span" helper method
  red_knot_python_semantic: move parameter span helper method
  ...
dcreager added a commit that referenced this pull request Apr 26, 2025
* main: (27 commits)
  [red-knot] Add new property tests for subtyping with "bottom" callable (#17635)
  [red-knot] Create generic context for generic classes lazily (#17617)
  ruff_db: add tests for annotations with no ranges
  [`airflow`] Extend `AIR301` rule (#17598)
  [`airflow`] update existing `AIR302` rules with better suggestions (#17542)
  red_knot_project: sort diagnostics from checking files
  [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621)
  [red-knot] fix inheritance-cycle detection for generic classes (#17620)
  [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411)
  Add Semantic Error Test for LateFutureImport (#17612)
  [red-knot] change TypeVarInstance to be interned, not tracked (#17616)
  [red-knot] Special case `@final`, `@override` (#17608)
  [red-knot] add TODO comment in specialization code (#17615)
  [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592)
  [syntax-errors] `nonlocal` declaration at module level (#17559)
  [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571)
  [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460)
  Bump 0.11.7 (#17613)
  [red-knot] Use iterative approach to collect overloads (#17607)
  red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants