Make SourceKind a required parameter
#7013
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
dhruvmanila
commented
Aug 31, 2023
Comment on lines
+174
to
+175
| let error_location = | ||
| if let Some(jupyter_index) = self.source_kind.as_ipy_notebook().map(Notebook::index) { |
There was a problem hiding this comment.
This is the main change in this section of the diff otherwise it's just a formatter change.
Comment on lines
-13
to
-21
| /// 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 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Not required because of is_macro::Is usage on the enum.
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
MichaReiser
reviewed
Aug 31, 2023
| error!("Failed to extract source from {}", path.display()); | ||
| return None; | ||
| }; | ||
| match add_noqa_to_path(path, package, &source_kind, source_type, settings) { |
There was a problem hiding this comment.
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.
dhruvmanila
force-pushed
the
dhruv/source-kind-required
branch
2 times, most recently
from
7017574 to
b369b12
Compare
charliermarsh
force-pushed
the
charlie/box-error
branch
from
b25806a to
e7a7910
Compare
charliermarsh
added a commit
that referenced
this pull request
Sep 1, 2023
…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
force-pushed
the
dhruv/source-kind-required
branch
from
b369b12 to
856f9c2
Compare
This was referenced Sep 1, 2023
dhruvmanila
force-pushed
the
dhruv/source-kind-required
branch
from
856f9c2 to
9c84798
Compare
MichaReiser
approved these changes
Sep 4, 2023
|
(Closing and re-opening the PR to trigger CI) |
This was referenced Sep 7, 2023
This was referenced Sep 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the
check_pathfunction to makeSourceKinda required parameter i.e., remove theOption<...>. The main blocker for this change was thatadd_noqawasn't supported for Jupyter Notebooks but it's still fine to pass in theSourceKindwithout it. This is ok because thePySourceTypeis 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,
SourceKindwill provide that but it was anOption<...>before.