Document LinterResult::has_syntax_error and add Parsed::has_no_errors
#16443
Conversation
ntBre
added
internal
|
…ors` Summary -- This is a follow up addressing the comments on #16425. As @dhruvmanila pointed out, the naming is a bit tricky. I went with `has_no_errors` to try to differentiate it from `is_valid`. It actually ends up negated in most uses, so it would be more convenient to have `has_any_errors` or `has_errors`, but I thought it would sound too much like the opposite of `is_valid` in that case. I'm definitely open to suggestions here. Test Plan -- Existing tests.
ntBre
force-pushed
the
brent/caching-followup
branch
from
b8a7569 to
fe3bcbd
Compare
crates/ruff_python_parser/src/lib.rs
Outdated
| @@ -346,10 +346,20 @@ impl<T> Parsed<T> { | |||
| /// | |||
| /// Note that this does not include version-related | |||
| /// [`unsupported_syntax_errors`](Parsed::unsupported_syntax_errors). | |||
| /// | |||
| /// See [`has_no_errors`](Parsed::has_no_errors) for a version that takes these into account. | |||
| pub fn is_valid(&self) -> bool { | |||
There was a problem hiding this comment.
Not specifically related to this PR but it seems a good opportunity. How about:
- Add a method
has_invalid_syntaxoris_invalid_syntaxas the opposite ofis_valid(we could also renameis_validtohas_valid_syntaxoris_valid_syntax. It's still somewhat ambigious by what it mean but I think it works well with unsupported syntax (which technically is valid, it's just unsupported) - Rename the new method to
has_syntax_errorsandhas_no_syntax_errors?
|
Thanks for the suggestions! There are now four methods here:
and I checked the references for each of the original methods and replaced negations with the new, negated methods. I also inlined the |
crates/ruff_python_parser/src/lib.rs
Outdated
| self.errors.is_empty() | ||
| } | ||
|
|
||
| /// Returns `true` if the parsed source code is invalid i.e., it has syntax errors. |
There was a problem hiding this comment.
I think i'd prefer to use the term parse errors or a more detailed description over using syntax errors. The description otherwise doesn't make it clear how it is different from has_syntax_erros.
But it's tricky with all those being syntax errors.
Another option is to rename unsupported_syntax_errors to has_unsupported_syntax (which would match has_invalid_syntax) or has_no_unsupported_syntax.
There was a problem hiding this comment.
Oh good catch. I can even link to the ParseError and UnsupportedSyntaxError types like I did in the new methods.
Summary
This is a follow up addressing the comments on #16425. As @dhruvmanila pointed out, the naming is a bit tricky. I went with
has_no_errorsto try to differentiate it fromis_valid. It actually ends up negated in most uses, so it would be more convenient to havehas_any_errorsorhas_errors, but I thought it would sound too much like the opposite ofis_validin that case. I'm definitely open to suggestions here.Test Plan
Existing tests.