Add a NotebookError type to avoid returning Diagnostics on error
#7035
Conversation
crates/ruff_cli/src/diagnostics.rs
Outdated
| }; | ||
|
|
||
| Ok(notebook) | ||
| fn notebook_from_path(path: &Path) -> Result<Option<Notebook>, NotebookError> { |
There was a problem hiding this comment.
Return Result<Option<Notebook>> instead of using empty Diagnostics to indicate the "we parsed the notebook, but it's not a Python notebook, so it should be skipped" case.
|
The |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
| TextRange::default(), | ||
| ) | ||
| })?; | ||
| reader.rewind()?; | ||
| let mut raw_notebook: RawNotebook = match serde_json::from_reader(reader.by_ref()) { | ||
| Ok(notebook) => notebook, | ||
| Err(err) => { | ||
| // Translate the error into a diagnostic |
There was a problem hiding this comment.
nit: we can remove this comment now that the error isn't being converted to a diagnostic
Yeah, that's nice. I think once we establish some abstraction around linting multiple filetypes it would be good to move this into it's own crate ( |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
charliermarsh
force-pushed
the
charlie/box-error
branch
from
b25806a
to
e7a7910
Compare
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 (indiagnostics.rs). This is more ergonomic, as errors can be composed and reported-on in different ways, whereas diagnostics require aPrinter, etc.See, e.g., #7013 (comment).
Test Plan
Ran
cargo runover a Python file labeled with a.ipynbsuffix, and saw: