From d0cd4aefd6c2a29523412385dc70518dc862122a Mon Sep 17 00:00:00 2001 From: augustelalande Date: Fri, 5 Apr 2024 22:05:22 -0400 Subject: [PATCH] implement write-whole-file --- .../resources/test/fixtures/refurb/FURB103.py | 123 ++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + .../rules/refurb/rules/write_whole_file.rs | 355 ++++++++++++++++++ ...es__refurb__tests__FURB103_FURB103.py.snap | 94 +++++ ruff.schema.json | 1 + scripts/add_rule.py | 7 +- 9 files changed, 583 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py new file mode 100644 index 0000000000000..d715dd8ef55ca --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py @@ -0,0 +1,123 @@ +def foo(): + ... + + +def bar(x): + ... + + +# Errors. + +# FURB103 +with open("file.txt", "w") as f: + f.write("test") + +# FURB103 +with open("file.txt", "wb") as f: + f.write(foobar) + +# FURB103 +with open("file.txt", mode="wb") as f: + f.write(b"abc") + +# FURB103 +with open("file.txt", "w", encoding="utf8") as f: + f.write(foobar) + +# FURB103 +with open("file.txt", "w", errors="ignore") as f: + f.write(foobar) + +# FURB103 +with open("file.txt", mode="w") as f: + f.write(foobar) + +# FURB103 +with open(foo(), "wb") as f: + # The body of `with` is non-trivial, but the recommendation holds. + bar("pre") + f.write(bar()) + bar("post") + print("Done") + +# FURB103 +with open("a.txt", "w") as a, open("b.txt", "wb") as b: + a.write(x) + b.write(y) + +# FURB103 +with foo() as a, open("file.txt", "w") as b, foo() as c: + # We have other things in here, multiple with items, but the user + # writes a single time to file and that bit they can replace. + bar(a) + b.write(bar(bar(a + x))) + bar(c) + + +# FURB103 +with open("file.txt", "w", newline="\r\n") as f: + f.write(foobar) + + +# Non-errors. + +with open("file.txt", errors="ignore", mode="wb") as f: + # Path.write_bytes() does not support errors + f.write(foobar) + +f2 = open("file2.txt", "w") +with open("file.txt", "w") as f: + f2.write(x) + +# mode is dynamic +with open("file.txt", foo()) as f: + f.write(x) + +# keyword mode is incorrect +with open("file.txt", mode="a+") as f: + f.write(x) + +# enables line buffering, not supported in write_text() +with open("file.txt", buffering=1) as f: + f.write(x) + +# dont mistake "newline" for "mode" +with open("file.txt", newline="wb") as f: + f.write(x) + +# I guess we can possibly also report this case, but the question +# is why the user would put "w+" here in the first place. +with open("file.txt", "w+") as f: + f.write(x) + +# Even though we write the whole file, we do other things. +with open("file.txt", "w") as f: + f.write(x) + f.seek(0) + x += f.read(100) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open(*filename, mode="w") as f: + f.write(x) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open(**kwargs) as f: + f.write(x) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open("file.txt", **kwargs) as f: + f.write(x) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open("file.txt", mode="w", **kwargs) as f: + f.write(x) + +# This could error (but doesn't), since it can't contain unsupported arguments, like +# `buffering`. +with open(*filename, mode="w") as f: + f.write(x) + +# This could error (but doesn't), since it can't contain unsupported arguments, like +# `buffering`. +with open(*filename, file="file.txt", mode="w") as f: + f.write(x) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 7e8293172916f..bff7e5658b6fb 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1217,6 +1217,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ReadWholeFile) { refurb::rules::read_whole_file(checker, with_stmt); } + if checker.enabled(Rule::WriteWholeFile) { + refurb::rules::write_whole_file(checker, with_stmt); + } if checker.enabled(Rule::UselessWithLock) { pylint::rules::useless_with_lock(checker, with_stmt); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 10a7fb752592f..c83ee76b3bef9 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1035,6 +1035,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // refurb (Refurb, "101") => (RuleGroup::Preview, rules::refurb::rules::ReadWholeFile), + (Refurb, "103") => (RuleGroup::Preview, rules::refurb::rules::WriteWholeFile), (Refurb, "105") => (RuleGroup::Preview, rules::refurb::rules::PrintEmptyString), #[allow(deprecated)] (Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index f27f2291d1135..37a91ca51dfbf 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -40,6 +40,7 @@ mod tests { #[test_case(Rule::MetaClassABCMeta, Path::new("FURB180.py"))] #[test_case(Rule::HashlibDigestHex, Path::new("FURB181.py"))] #[test_case(Rule::ListReverseCopy, Path::new("FURB187.py"))] + #[test_case(Rule::WriteWholeFile, Path::new("FURB103.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index c18f2350ecd4d..34d760b48dd05 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -24,6 +24,7 @@ pub(crate) use type_none_comparison::*; pub(crate) use unnecessary_enumerate::*; pub(crate) use unnecessary_from_float::*; pub(crate) use verbose_decimal_constructor::*; +pub(crate) use write_whole_file::*; mod bit_count; mod check_and_remove_from_set; @@ -51,3 +52,4 @@ mod type_none_comparison; mod unnecessary_enumerate; mod unnecessary_from_float; mod verbose_decimal_constructor; +mod write_whole_file; diff --git a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs new file mode 100644 index 0000000000000..066e1fb737d22 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -0,0 +1,355 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::visitor::{self, Visitor}; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_codegen::Generator; +use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; +use crate::fix::snippet::SourceCodeSnippet; + +/// ## What it does +/// Checks for uses of `open` and `write` that can be replaced by `pathlib` +/// methods, like `Path.write_text` and `Path.write_bytes`. +/// +/// ## Why is this bad? +/// When writing a single string to a file, it's simpler and more concise +/// to use `pathlib` methods like `Path.write_text` and `Path.write_bytes` +/// instead of `open` and `write` calls via `with` statements. +/// +/// ## Example +/// ```python +/// with open(filename, "w") as f: +/// f.write(contents) +/// ``` +/// +/// Use instead: +/// ```python +/// from pathlib import Path +/// +/// Path(filename).write_text(contents) +/// ``` +/// +/// ## References +/// - [Python documentation: `Path.write_bytes`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_bytes) +/// - [Python documentation: `Path.write_text`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_text) +#[violation] +pub struct WriteWholeFile { + filename: SourceCodeSnippet, + suggestion: SourceCodeSnippet, +} + +impl Violation for WriteWholeFile { + #[derive_message_formats] + fn message(&self) -> String { + let filename = self.filename.truncated_display(); + let suggestion = self.suggestion.truncated_display(); + format!("`open` and `write` should be replaced by `Path({filename}).{suggestion}`") + } +} + +/// FURB103 +pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { + // `async` check here is more of a precaution. + if with.is_async || !checker.semantic().is_builtin("open") { + return; + } + + // First we go through all the items in the statement and find all `open` operations. + let candidates = find_file_opens(with, checker.semantic()); + if candidates.is_empty() { + return; + } + + // Then we need to match each `open` operation with exactly one `write` call. + let mut matcher = WriteMatcher::new(candidates); + visitor::walk_body(&mut matcher, &with.body); + let contents = matcher.contents(); + + // All the matched operations should be reported. + let diagnostics: Vec = matcher + .into_matches() + .iter() + .zip(contents) + .map(|(open, content)| { + Diagnostic::new( + WriteWholeFile { + filename: SourceCodeSnippet::from_str(&checker.generator().expr(open.filename)), + suggestion: make_suggestion(open, content, checker.generator()), + }, + open.item.range(), + ) + }) + .collect(); + checker.diagnostics.extend(diagnostics); +} + +#[derive(Debug)] +enum WriteMode { + /// "w" -> `write_text` + Text, + /// "wb" -> `write_bytes` + Bytes, +} + +/// A grab bag struct that joins together every piece of information we need to track +/// about a file open operation. +#[derive(Debug)] +struct FileOpen<'a> { + /// With item where the open happens, we use it for the reporting range. + item: &'a ast::WithItem, + /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. + filename: &'a Expr, + /// The type of write to choose `write_text` or `write_bytes`. + mode: WriteMode, + /// Keywords that can be used in the new write call. + keywords: Vec<&'a ast::Keyword>, + /// We only check `open` operations whose file handles are used exactly once. + reference: &'a ResolvedReference, +} + +impl<'a> FileOpen<'a> { + /// Determine whether an expression is a reference to the file handle, by comparing + /// their ranges. If two expressions have the same range, they must be the same expression. + fn is_ref(&self, expr: &Expr) -> bool { + expr.range() == self.reference.range() + } +} + +/// Find and return all `open` operations in the given `with` statement. +fn find_file_opens<'a>( + with: &'a ast::StmtWith, + semantic: &'a SemanticModel<'a>, +) -> Vec> { + with.items + .iter() + .filter_map(|item| find_file_open(item, with, semantic)) + .collect() +} + +/// Find `open` operation in the given `with` item. +fn find_file_open<'a>( + item: &'a ast::WithItem, + with: &'a ast::StmtWith, + semantic: &'a SemanticModel<'a>, +) -> Option> { + // We want to match `open(...) as var`. + let ast::ExprCall { + func, + arguments: ast::Arguments { args, keywords, .. }, + .. + } = item.context_expr.as_call_expr()?; + + if func.as_name_expr()?.id != "open" { + return None; + } + + let var = item.optional_vars.as_deref()?.as_name_expr()?; + + // Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="w")`, + // it could be a match; but in all other cases, the call _could_ contain unsupported keyword + // arguments, like `buffering`. + if args.iter().any(Expr::is_starred_expr) + || keywords.iter().any(|keyword| keyword.arg.is_none()) + { + return None; + } + + // Match positional arguments, get filename and mode. + let (filename, pos_mode) = match_open_args(args)?; + + // Match keyword arguments, get keyword arguments to forward and possibly mode. + let (keywords, kw_mode) = match_open_keywords(keywords)?; + + let mode = match kw_mode { + Some(kw) => kw, + None => match pos_mode { + Some(pos) => pos, + None => return None, // Indicates default mode "r", which is incompatible. + }, + }; + + // Path.write_bytes does not support any kwargs. + if matches!(mode, WriteMode::Bytes) && !keywords.is_empty() { + return None; + } + + // Now we need to find what is this variable bound to... + let scope = semantic.current_scope(); + let bindings: Vec = scope.get_all(var.id.as_str()).collect(); + + let binding = bindings + .iter() + .map(|x| semantic.binding(*x)) + // We might have many bindings with the same name, but we only care + // for the one we are looking at right now. + .find(|binding| binding.range() == var.range())?; + + // Since many references can share the same binding, we can limit our attention span + // exclusively to the body of the current `with` statement. + let references: Vec<&ResolvedReference> = binding + .references + .iter() + .map(|id| semantic.reference(*id)) + .filter(|reference| with.range().contains_range(reference.range())) + .collect(); + + // And even with all these restrictions, if the file handle gets used not exactly once, + // it doesn't fit the bill. + let [reference] = references.as_slice() else { + return None; + }; + + Some(FileOpen { + item, + filename, + mode, + keywords, + reference, + }) +} + +/// Match positional arguments. Return expression for the file name and mode. +fn match_open_args(args: &[Expr]) -> Option<(&Expr, Option)> { + match args { + // If only one argument indicating read mode, the mode could still be set as a kwarg. + [filename] => Some((filename, None)), + [filename, mode_literal] => { + match_open_mode(mode_literal).map(|mode| (filename, Some(mode))) + } + // The third positional argument is `buffering` and `write_text` doesn't support it. + _ => None, + } +} + +/// Match keyword arguments. Return keyword arguments to forward and mode. +fn match_open_keywords( + keywords: &[ast::Keyword], +) -> Option<(Vec<&ast::Keyword>, Option)> { + let mut result: Vec<&ast::Keyword> = vec![]; + let mut mode: Option = None; + + for keyword in keywords { + match keyword.arg.as_ref()?.as_str() { + "encoding" | "errors" | "newline" => result.push(keyword), + + // This might look bizarre, - why do we re-wrap this optional? + // + // The answer is quite simple, in the result of the current function + // mode being `None` is a possible and correct option meaning that there + // was NO "mode" keyword argument. + // + // The result of `match_open_mode` on the other hand is None + // in the cases when the mode is not compatible with `write_text`/`write_bytes`. + // + // So, here we return None from this whole function if the mode + // is incompatible. + "mode" => mode = Some(match_open_mode(&keyword.value)?), + + // All other keywords cannot be directly forwarded. + _ => return None, + }; + } + Some((result, mode)) +} + +/// Match open mode to see if it is supported. +fn match_open_mode(mode: &Expr) -> Option { + let ast::ExprStringLiteral { value, .. } = mode.as_string_literal_expr()?; + if value.is_implicit_concatenated() { + return None; + } + match value.to_str() { + "w" => Some(WriteMode::Text), + "wb" => Some(WriteMode::Bytes), + _ => None, + } +} + +/// AST visitor that matches `open` operations with the corresponding `write` calls. +#[derive(Debug)] +struct WriteMatcher<'a> { + candidates: Vec>, + matches: Vec>, + contents: Vec>, +} + +impl<'a> WriteMatcher<'a> { + fn new(candidates: Vec>) -> Self { + Self { + candidates, + matches: vec![], + contents: vec![], + } + } + + fn into_matches(self) -> Vec> { + self.matches + } + + fn contents(&self) -> Vec> { + self.contents.clone() + } +} + +impl<'a> Visitor<'a> for WriteMatcher<'a> { + fn visit_expr(&mut self, expr: &'a Expr) { + if let Some((write_to, content)) = match_write_call(expr) { + if let Some(open) = self + .candidates + .iter() + .position(|open| open.is_ref(write_to)) + { + self.matches.push(self.candidates.remove(open)); + self.contents.push(content); + } + return; + } + visitor::walk_expr(self, expr); + } +} + +/// Match `x.write(foo)` expression and return expression `x` and `foo` on success. +fn match_write_call(expr: &Expr) -> Option<(&Expr, Vec)> { + let call = expr.as_call_expr()?; + let attr = call.func.as_attribute_expr()?; + let method_name = &attr.attr; + + if method_name != "write" + || !attr.value.is_name_expr() + || call.arguments.args.len() != 1 + || !call.arguments.keywords.is_empty() + { + return None; + } + + Some((attr.value.as_ref(), call.arguments.args.as_ref().to_vec())) +} + +/// Construct the replacement suggestion call. +fn make_suggestion( + open: &FileOpen<'_>, + write_arguments: Vec, + generator: Generator, +) -> SourceCodeSnippet { + let method_name = match open.mode { + WriteMode::Text => "write_text", + WriteMode::Bytes => "write_bytes", + }; + let name = ast::ExprName { + id: method_name.to_string(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + let call = ast::ExprCall { + func: Box::new(name.into()), + arguments: ast::Arguments { + args: Box::from(write_arguments), + keywords: open.keywords.iter().copied().cloned().collect(), + range: TextRange::default(), + }, + range: TextRange::default(), + }; + SourceCodeSnippet::from_str(&generator.expr(&call.into())) +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap new file mode 100644 index 0000000000000..9a37a5447d495 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap @@ -0,0 +1,94 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB103.py:12:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text("test")` + | +11 | # FURB103 +12 | with open("file.txt", "w") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +13 | f.write("test") + | + +FURB103.py:16:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_bytes(foobar)` + | +15 | # FURB103 +16 | with open("file.txt", "wb") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +17 | f.write(foobar) + | + +FURB103.py:20:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_bytes(b"abc")` + | +19 | # FURB103 +20 | with open("file.txt", mode="wb") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +21 | f.write(b"abc") + | + +FURB103.py:24:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(encoding="utf8", foobar)` + | +23 | # FURB103 +24 | with open("file.txt", "w", encoding="utf8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +25 | f.write(foobar) + | + +FURB103.py:28:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(errors="ignore", foobar)` + | +27 | # FURB103 +28 | with open("file.txt", "w", errors="ignore") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +29 | f.write(foobar) + | + +FURB103.py:32:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(foobar)` + | +31 | # FURB103 +32 | with open("file.txt", mode="w") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +33 | f.write(foobar) + | + +FURB103.py:36:6: FURB103 `open` and `write` should be replaced by `Path(foo()).write_bytes(bar())` + | +35 | # FURB103 +36 | with open(foo(), "wb") as f: + | ^^^^^^^^^^^^^^^^^^^^^^ FURB103 +37 | # The body of `with` is non-trivial, but the recommendation holds. +38 | bar("pre") + | + +FURB103.py:44:6: FURB103 `open` and `write` should be replaced by `Path("a.txt").write_text(x)` + | +43 | # FURB103 +44 | with open("a.txt", "w") as a, open("b.txt", "wb") as b: + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +45 | a.write(x) +46 | b.write(y) + | + +FURB103.py:44:31: FURB103 `open` and `write` should be replaced by `Path("b.txt").write_bytes(y)` + | +43 | # FURB103 +44 | with open("a.txt", "w") as a, open("b.txt", "wb") as b: + | ^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +45 | a.write(x) +46 | b.write(y) + | + +FURB103.py:49:18: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(bar(bar(a + x)))` + | +48 | # FURB103 +49 | with foo() as a, open("file.txt", "w") as b, foo() as c: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +50 | # We have other things in here, multiple with items, but the user +51 | # writes a single time to file and that bit they can replace. + | + +FURB103.py:58:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(newline="\r\n", foobar)` + | +57 | # FURB103 +58 | with open("file.txt", "w", newline="\r\n") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +59 | f.write(foobar) + | diff --git a/ruff.schema.json b/ruff.schema.json index 4aa04519ad805..ab68b675b3bd1 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3041,6 +3041,7 @@ "FURB1", "FURB10", "FURB101", + "FURB103", "FURB105", "FURB11", "FURB113", diff --git a/scripts/add_rule.py b/scripts/add_rule.py index bfc61b733e8ae..d324e8001b4e5 100755 --- a/scripts/add_rule.py +++ b/scripts/add_rule.py @@ -9,6 +9,7 @@ --code 807 \ --linter flake8-pie """ + from __future__ import annotations import argparse @@ -26,9 +27,7 @@ def main(*, name: str, prefix: str, code: str, linter: str) -> None: / "crates/ruff_linter/resources/test/fixtures" / dir_name(linter) / f"{filestem}.py" - ).open( - "a", - ): + ).open("a"): pass plugin_module = ROOT_DIR / "crates/ruff_linter/src/rules" / dir_name(linter) @@ -140,7 +139,7 @@ def main(*, name: str, prefix: str, code: str, linter: str) -> None: variant = pascal_case(linter) rule = f"""rules::{linter.split(" ")[0]}::rules::{name}""" lines.append( - " " * 8 + f"""({variant}, "{code}") => (RuleGroup::Stable, {rule}),\n""", + " " * 8 + f"""({variant}, "{code}") => (RuleGroup::Preview, {rule}),\n""", ) lines.sort() text += "".join(lines)