Skip to content

[flake8-errmsg] Extend EM101 to support byte strings #18867

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

njhearp
Copy link
Contributor

@njhearp njhearp commented Jun 22, 2025

Summary

Fixes #18765

Test Plan

Added test

@njhearp njhearp changed the title Extended EM101 to support byte strings [flake8-errmsg] Extended EM101 to support byte strings Jun 22, 2025
Copy link
Contributor

github-actions bot commented Jun 22, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@njhearp
Copy link
Contributor Author

njhearp commented Jun 23, 2025

Following the recommendation from the error got the CI tests to pass. I'm just a little concerned about the initial failure.

@MichaReiser
Copy link
Member

Following the recommendation from the error got the CI tests to pass. I'm just a little concerned about the initial failure.

What was the initial failure?

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jun 23, 2025
@@ -203,6 +203,26 @@ pub(crate) fn string_in_exception(checker: &Checker, stmt: &Stmt, exc: &Expr) {
}
}
}
// Check for byte string literals.
Expr::BytesLiteral(ast::ExprBytesLiteral { value: bytes, .. }) => {
if checker.settings.rules.enabled(Rule::RawStringInException) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should gate this change behind preview as it is an extension of a stable rule

See

pub(crate) const fn is_unicode_to_unicode_confusables_enabled(settings: &LinterSettings) -> bool {
for other cases where we gated behavior behind a preview flag.

@njhearp
Copy link
Contributor Author

njhearp commented Jun 23, 2025

error[E0599]: no method named `enabled` found for reference `&Checker<'_>` in the current scope
   --> crates/ruff_linter/src/rules/flake8_errmsg/rules/string_in_exception.rs:208:32
    |
208 |                     if checker.enabled(Rule::RawStringInException) {
    |                                ^^^^^^^ method not found in `&Checker<'_>`
    |
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following traits define an item `enabled`, perhaps you need to implement one of them:
            candidate #1: `Log`
            candidate #2: `tracing_core::subscriber::Subscriber`
            candidate #3: `tracing_subscriber::layer::Filter`
            candidate #4: `tracing_subscriber::layer::Layer`
help: one of the expressions' fields has a method of the same name
    |
208 |                     if checker.settings.rules.enabled(Rule::RawStringInException) {
    |                                +++++++++++++++

The CI error suggested this. I believe it is because of Rust's ownership; it only flagged line 208. I just wanted to make sure there was no problem with it.

@ntBre
Copy link
Contributor

ntBre commented Jun 23, 2025

You may just need to rebase or merge main. We recently renamed Checker::enabled to Checker::is_rule_enabled and also made the settings field a method.

Copy link
Contributor

github-actions bot commented Jun 24, 2025

mypy_primer results

No ecosystem changes detected ✅

@njhearp
Copy link
Contributor Author

njhearp commented Jun 24, 2025

I updated it so the change is gated behind the preview flag. Also, I merged with main. Sorry about the mess with the history; I realized it would have been cleaner to rebase.

@MichaReiser
Copy link
Member

@njhearp I think something went wrong with your merge. The diff contains too many unrelated changes (which should already be on main?). I'm not quite sure what went wrong here. Could you try doing an interactive rebase and only pick your commits?

@AlexWaygood AlexWaygood removed their request for review June 24, 2025 07:31
@njhearp njhearp force-pushed the EM101-byte-string branch from 7e25e51 to 2253444 Compare June 25, 2025 02:35
@MichaReiser MichaReiser requested a review from ntBre June 25, 2025 06:56
@ntBre ntBre added the preview Related to preview mode features label Jun 25, 2025
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@ntBre ntBre changed the title [flake8-errmsg] Extended EM101 to support byte strings [flake8-errmsg] Extend EM101 to support byte strings Jun 25, 2025
@ntBre ntBre merged commit 8b22992 into astral-sh:main Jun 25, 2025
36 checks passed
dcreager added a commit that referenced this pull request Jun 26, 2025
* main:
  [ty] Add regression-benchmark for attribute-assignment hang (#18957)
  [ty] Format conflicting types as an enumeration (#18956)
  [ty] Prevent union builder construction for just one declaration (#18954)
  [ty] Infer nonlocal types as unions of all reachable bindings (#18750)
  [`pyflakes`] Mark `F504`/`F522`/`F523` autofix as unsafe if there's a call with side effect (#18839)
  [`playground`] Add ruff logo docs link to Header.tsx (#18947)
  [ty] Reduce the overwhelming complexity of `TypeInferenceBuilder::infer_call_expression` (#18943)
  [ty] Add subdiagnostic about empty bodies in more cases (#18942)
  [ty] Move search path resolution to `Options::to_program_settings` (#18937)
  [`flake8-errmsg`] Extend `EM101` to support byte strings (#18867)
  Move big rule implementations (#18931)
  [`pylint`] Allow fix with comments and document performance implications (`PLW3301`) (#18936)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EM101 could support byte strings
3 participants