Document LinterResult::has_syntax_error and add Parsed::has_no_errors#16443
Document LinterResult::has_syntax_error and add Parsed::has_no_errors#16443
LinterResult::has_syntax_error and add Parsed::has_no_errors#16443Conversation
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
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Thanks for doing this.
Regardless of the name choice, I think it might be useful to keep it consistent throughout the code base i.e., keep the method name same as variable name and field name on LinterResult
crates/ruff_python_parser/src/lib.rs
Outdated
| /// [`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.
3 participants
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.