Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
dhruvmanila
force-pushed
the
dhruv/string-like
branch
2 times, most recently
from
74f2873 to
0e1afa4
Compare
|
dhruvmanila
force-pushed
the
dhruv/fstring-element
branch
from
bd821b8 to
b6be853
Compare
dhruvmanila
force-pushed
the
dhruv/string-like
branch
2 times, most recently
from
8e34f89 to
a6e5ce3
Compare
| } | ||
| Expr::BytesLiteral(bytes_literal) => analyze::string_like(bytes_literal.into(), self), | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
Maybe just embed this in analyze::expression rather than as a separate call here?
There was a problem hiding this comment.
This might be silly but wouldn't that mix the concerns? I'm fine either way.
charliermarsh
left a comment
charliermarsh
left a comment
There was a problem hiding this comment.
No strong opinion, seems reasonable.
| /// This includes string literals, bytes literals, and the literal parts of | ||
| /// f-strings. | ||
| #[derive(Copy, Clone, Debug, PartialEq)] | ||
| pub enum StringLike<'a> { |
There was a problem hiding this comment.
Should StringLike have an as_str() method for easy comparision?
There was a problem hiding this comment.
I don't think so as it also contains bytes literals and implicitly concatenated strings which can possibly do an allocation.
dhruvmanila
force-pushed
the
dhruv/string-like
branch
from
a6e5ce3 to
1b2120b
Compare
dhruvmanila
force-pushed
the
dhruv/string-like
branch
from
1b2120b to
6fc3414
Compare
3 participants
Summary
This PR introduces a new
StringLikeenum which is a narrow type to indicate string-like nodes. These includes the string literals, bytes literals, and the literal parts of f-strings.The main motivation behind this is to avoid repetition of rule calling in the AST checker. We add a new
analyze::string_likefunction which takes in the enum and calls all the respective rule functions which expects atleast 2 of the variants of this enum.I'm open to discarding this if others think it's not that useful at this stage as currently only 3 rules require these nodes.
As suggested here and here.
Test Plan
cargo test