Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce StringLike enum #9016

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Introduce StringLike enum #9016

merged 3 commits into from
Dec 7, 2023

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Dec 5, 2023

Summary

This PR introduces a new StringLike enum 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_like function 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

@dhruvmanila dhruvmanila added the internal An internal refactor or improvement label Dec 5, 2023
@dhruvmanila dhruvmanila force-pushed the dhruv/string-like branch 2 times, most recently from 74f2873 to 0e1afa4 Compare December 5, 2023 20:40
Copy link
Contributor

github-actions bot commented Dec 5, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@dhruvmanila dhruvmanila force-pushed the dhruv/string-like branch 2 times, most recently from 8e34f89 to a6e5ce3 Compare December 6, 2023 17:56
@dhruvmanila dhruvmanila marked this pull request as ready for review December 6, 2023 21:52
}
Expr::BytesLiteral(bytes_literal) => analyze::string_like(bytes_literal.into(), self),
_ => {}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just embed this in analyze::expression rather than as a separate call here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be silly but wouldn't that mix the concerns? I'm fine either way.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should StringLike have an as_str() method for easy comparision?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so as it also contains bytes literals and implicitly concatenated strings which can possibly do an allocation.

Base automatically changed from dhruv/fstring-element to main December 7, 2023 16:28
@dhruvmanila dhruvmanila enabled auto-merge (squash) December 7, 2023 16:30
@dhruvmanila dhruvmanila merged commit 96ae9fe into main Dec 7, 2023
16 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/string-like branch December 7, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants