Make `SourceKind` a required parameter by dhruvmanila · Pull Request #7013 · astral-sh/ruff

dhruvmanila · 2023-08-31T03:06:55Z

Summary

This PR refactors the check_path function to make SourceKind a required parameter i.e., remove the Option<...>. The main blocker for this change was that add_noqa wasn't supported for Jupyter Notebooks but it's still fine to pass in the SourceKind without it. This is ok because the PySourceType is checked before and we don't proceed unless it's either a Python or stub file.

This is also required for the PEP 701 f-string parser changes because we want the source code to be available while parsing to extract the leading and trailing text for debug expressions. Here, SourceKind will provide that but it was an Option<...> before.

dhruvmanila · 2023-08-31T03:07:06Z

Current dependencies on/for this PR:

main
- PR Add a NotebookError type to avoid returning Diagnostics on error #7035
  - PR Make SourceKind a required parameter #7013 👈
    - PR Add support for the new f-string tokens per PEP 701 #6659
      - PR Add support for parsing f-string as per PEP 701 #7041
        
        PR Use narrow type for string parsing patterns #7211
        
        PR Disallow non-parenthesized lambda expr in f-string #7263
        
        PR Fix curly brace escape handling in f-strings #7331
        
        PR Update Indexer to use new f-string tokens #7325
        
        PR Detect noqa directives for multi-line f-strings #7326
        PR Update F541 to use new f-string tokens #7327
        PR Update Stylist quote detection with new f-string token #7328
        PR Update W605 to check in f-strings #7329

This comment was auto-generated by Graphite.

dhruvmanila · 2023-08-31T03:12:43Z

crates/ruff/src/logging.rs

+        let error_location =
+            if let Some(jupyter_index) = self.source_kind.as_ipy_notebook().map(Notebook::index) {


This is the main change in this section of the diff otherwise it's just a formatter change.

dhruvmanila · 2023-08-31T03:13:17Z

crates/ruff/src/source_kind.rs

-    /// Return the [`Notebook`] if the source kind is [`SourceKind::IpyNotebook`].
-    pub fn notebook(&self) -> Option<&Notebook> {
-        if let Self::IpyNotebook(notebook) = self {
-            Some(notebook)
-        } else {
-            None
-        }
-    }
-


Not required because of is_macro::Is usage on the enum.

crates/ruff_cli/src/commands/add_noqa.rs

github-actions · 2023-08-31T03:20:16Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

crates/ruff/src/linter.rs

crates/ruff_cli/src/commands/add_noqa.rs

MichaReiser · 2023-08-31T06:09:50Z

crates/ruff_cli/src/commands/add_noqa.rs

+                error!("Failed to extract source from {}", path.display());
+                return None;
+            };
+            match add_noqa_to_path(path, package, &source_kind, source_type, settings) {


Maybe unify the error handling either by using LintSource::try_from_path(...).and_then(|source| add_noqa_to_path(...)) or extracing into its own function and calling it.

…7035) ## Summary This PR refactors the error-handling cases around Jupyter notebooks to use errors rather than `Box<Diagnostics>`, which creates some oddities in the downstream handling. So, instead of formatting errors as diagnostics _eagerly_ (in the notebook methods), we now return errors and convert those errors to diagnostics at the last possible moment (in `diagnostics.rs`). This is more ergonomic, as errors can be composed and reported-on in different ways, whereas diagnostics require a `Printer`, etc. See, e.g., #7013 (comment). ## Test Plan Ran `cargo run` over a Python file labeled with a `.ipynb` suffix, and saw: ``` foo.ipynb:1:1: E999 SyntaxError: Expected a Jupyter Notebook, which must be internally stored as JSON, but found a Python source file: expected value at line 1 column 1 ```

dhruvmanila · 2023-09-04T07:35:33Z

(Closing and re-opening the PR to trigger CI)

dhruvmanila commented Aug 31, 2023

View reviewed changes

dhruvmanila requested a review from MichaReiser August 31, 2023 03:16

MichaReiser reviewed Aug 31, 2023

View reviewed changes

MichaReiser requested a review from charliermarsh August 31, 2023 06:10

MichaReiser added the internal An internal refactor or improvement label Aug 31, 2023

charliermarsh mentioned this pull request Aug 31, 2023

Add a NotebookError type to avoid returning Diagnostics on error #7035

Merged

dhruvmanila changed the base branch from main to charlie/box-error September 1, 2023 05:04

dhruvmanila force-pushed the dhruv/source-kind-required branch 2 times, most recently from 7017574 to b369b12 Compare September 1, 2023 05:06

charliermarsh force-pushed the charlie/box-error branch from b25806a to e7a7910 Compare September 1, 2023 10:56

Base automatically changed from charlie/box-error to main September 1, 2023 11:08

dhruvmanila force-pushed the dhruv/source-kind-required branch from b369b12 to 856f9c2 Compare September 1, 2023 13:05

This was referenced Sep 1, 2023

Add support for the new f-string tokens per PEP 701 #6659

Merged

Add support for parsing f-string as per PEP 701 #7041

Merged

dhruvmanila requested a review from MichaReiser September 1, 2023 13:22

dhruvmanila added 2 commits September 3, 2023 09:13

Make SourceKind a required parameter

e1b1c97

Use the new error structure

9c84798

dhruvmanila force-pushed the dhruv/source-kind-required branch from 856f9c2 to 9c84798 Compare September 3, 2023 07:04

MichaReiser approved these changes Sep 4, 2023

View reviewed changes

dhruvmanila closed this Sep 4, 2023

dhruvmanila reopened this Sep 4, 2023

dhruvmanila enabled auto-merge (squash) September 4, 2023 07:36

dhruvmanila merged commit 1067261 into main Sep 4, 2023

dhruvmanila deleted the dhruv/source-kind-required branch September 4, 2023 07:46

This was referenced Sep 7, 2023

Use narrow type for string parsing patterns #7211

Merged

Disallow non-parenthesized lambda expr in f-string #7263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Make `SourceKind` a required parameter#7013

Make `SourceKind` a required parameter#7013
dhruvmanila merged 2 commits intomainfrom
dhruv/source-kind-required

dhruvmanila commented Aug 31, 2023 •

edited

Loading

Uh oh!

dhruvmanila commented Aug 31, 2023 •

edited

Loading

Uh oh!

dhruvmanila Aug 31, 2023

Uh oh!

dhruvmanila Aug 31, 2023

Uh oh!

Uh oh!

github-actions bot commented Aug 31, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

MichaReiser Aug 31, 2023

Uh oh!

dhruvmanila commented Sep 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		let error_location =
		if let Some(jupyter_index) = self.source_kind.as_ipy_notebook().map(Notebook::index) {

Comments

Conversation

dhruvmanila commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

dhruvmanila commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhruvmanila Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Uh oh!

Uh oh!

Uh oh!

MichaReiser Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

dhruvmanila commented Sep 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dhruvmanila commented Aug 31, 2023 •

edited

Loading

dhruvmanila commented Aug 31, 2023 •

edited

Loading

github-actions bot commented Aug 31, 2023 •

edited

Loading