nitpicks by ShaharNaveh · Pull Request #5886 · RustPython/RustPython

32-32: LGTM: Good use of Self shorthand.

Using Self instead of the explicit type name follows Rust best practices for constructor methods.

89-89: LGTM: Consistent use of Self shorthand.

This change maintains consistency with the stylistic improvements throughout the codebase.

139-139: LGTM: Proper use of Self in tuple struct constructor.

The Self shorthand correctly applies to tuple struct construction as well.

7-8: LGTM: Excellent use of Self for enum variants.

Using Self::And and Self::Or instead of the full enum path improves readability and follows Rust idioms for enum variant references within impl blocks.

Also applies to: 23-25

39-51: LGTM: Consistent enum variant refactoring.

The systematic replacement of ruff::Operator::* with Self::* throughout both the ast_to_object and ast_from_object methods improves code conciseness while maintaining functionality.

Also applies to: 66-90

104-107: LGTM: Proper Self usage for UnaryOp variants.

The changes maintain consistency with the refactoring pattern applied to other enum implementations in this file.

Also applies to: 122-128

142-151: LGTM: Complete and consistent CmpOp refactoring.

All comparison operator variants correctly use the Self:: shorthand, completing the stylistic improvements for this enum's Node trait implementation.

Also applies to: 166-184

45-45: LGTM: Improved readability with blank line.

Adding a blank line after the type definition improves code structure and readability.

1220-1220: LGTM: Consistent formatting improvements.

The added blank lines after method implementations enhance code readability and provide better visual separation between different trait implementations.

Also applies to: 1230-1230, 1250-1250, 1344-1344, 1397-1397

1281-1282: LGTM: Cleaner string literals with raw strings.

Converting to raw string literals (r#""#) improves readability for strings containing quotes and escape sequences, making the error messages clearer to maintain.

Also applies to: 1293-1294, 1412-1413, 1424-1425

394-394: LGTM! String allocation optimization approved.

Replacing String::from() with a string literal eliminates unnecessary heap allocation for the error message, improving performance.

402-402: LGTM! String allocation optimization approved.

Replacing String::from() with a string literal eliminates unnecessary heap allocation for the error message, improving performance.

591-591: LGTM! Readability improvements approved.

Adding blank lines after function definitions improves code readability and follows Rust formatting conventions.

Also applies to: 607-607, 608-608, 609-609, 619-619, 624-624, 625-625, 629-629, 633-633, 642-642

670-674: LGTM! Using Self improves maintainability.

Using Self instead of the explicit type name SchedParam is a Rust best practice that improves code maintainability and reduces refactoring overhead.

710-718: LGTM! Using Self improves maintainability.

Using Self instead of the explicit type name SchedParam is a Rust best practice that improves code maintainability and reduces refactoring overhead.

1767-1767: LGTM! String allocation optimization approved.

Replacing .to_owned() with a string literal eliminates unnecessary heap allocation for the error message, improving performance.

33-35: LGTM! const fn annotation approved.

Adding const fn to getcodesize() enables compile-time evaluation of this simple constant-returning function, which is a good optimization.

39-39: LGTM! Improved readability with range pattern matching.

Using (b'a' as i32..=b'z' as i32).contains(&ch) is more idiomatic and readable than manual range checks, while maintaining the same logic.

409-411: LGTM! const fn annotation approved.

Adding const fn to the groups() getter method enables compile-time evaluation of this simple field access, which is appropriate for a const getter.

641-643: LGTM! const fn annotations approved.

Adding const fn to these simple getter methods (pos(), endpos(), lastindex()) enables compile-time evaluation and is appropriate for these const accessors.

Also applies to: 646-648, 651-657

724-724: LGTM! String allocation optimization approved.

Replacing .to_owned() with a string literal eliminates unnecessary heap allocation for the error message, improving performance.

749-749: LGTM! String allocation optimization approved.

Replacing .to_owned() with a string literal eliminates unnecessary heap allocation for the error message, improving performance.

36-36: LGTM! Readability improvements approved.

Adding blank lines improves code readability and follows Rust formatting conventions. The trailing newline at the end of the file is also a good practice.

Also applies to: 41-41, 47-47, 52-52, 355-355, 402-402, 407-407, 412-412, 658-658, 664-664, 669-669, 674-674, 687-687, 692-692

62-64: LGTM! Appropriate const fn conversions.

These method conversions to const fn are well-suited as they only return primitive values, boolean flags, or simple field access without side effects. This enables compile-time evaluation and aligns with Rust's const-correctness principles.

Also applies to: 67-69, 134-136, 178-180, 188-190, 199-201, 204-206, 209-211, 214-216, 219-221, 224-226, 229-231

58-58: LGTM! Good use of Self shorthand.

Using Self { expression, range } instead of the explicit struct name improves maintainability and follows Rust conventions.

81-81: LGTM! Improved code readability.

The added blank lines improve separation between methods and enhance code readability.

Also applies to: 100-100, 127-127

11-11: LGTM! Improved trait method separation.

The added blank line enhances readability by providing better visual separation between trait methods.

48-48: LGTM! More idiomatic Self usage.

Using Self::new instead of Box::new is more idiomatic and maintains consistency with similar patterns throughout the codebase.

46-46: LGTM! Enhanced code readability.

The added blank lines improve visual separation between methods and enhance overall code readability.

Also applies to: 99-99, 130-130

55-55: LGTM! Consistent formatting improvement.

The added blank line maintains consistency with the formatting improvements applied across the AST module and enhances method separation.

61-61: LGTM! Consistent Self shorthand usage.

Using Self { instead of the explicit struct name aligns with the broader refactoring pattern applied across the AST module and improves maintainability.

67-67: Excellent use of Self for improved readability.

The replacement of explicit type names like ruff::ModModule with Self follows Rust best practices and makes the code more concise and maintainable.

Also applies to: 96-96, 149-149, 185-185, 211-211

36-36: Good addition of blank lines for better code organization.

The blank lines after method implementations improve readability by clearly separating different implementation blocks.

Also applies to: 63-63, 90-90, 129-129, 145-145, 159-159, 205-205

58-58: Improved string literal clarity with raw strings.

Using raw string literals r#""# instead of the format! macro makes the error messages more readable and eliminates the need for escaping quotes.

Also applies to: 79-79

110-110: Excellent conversion to const fn.

Making get_one_indexed a const fn enables compile-time evaluation, which can improve performance and follows modern Rust practices for simple accessor methods.

53-60: Consistent use of Self improves code maintainability.

The systematic replacement of ruff::Pattern:: with Self:: in pattern matching makes the code more concise and easier to refactor while maintaining the same functionality.

Also applies to: 70-76, 82-88, 94-100, 106-112

24-24: Improved code organization with blank lines.

The addition of blank lines after method implementations enhances readability by clearly separating different implementation blocks.

Also applies to: 48-48, 141-141, 157-157, 177-177, 193-193, 207-207, 227-227, 243-243, 269-269, 293-293, 322-322, 430-430, 444-444, 464-464, 481-481

10-44: Comprehensive use of Self enhances code consistency.

The systematic replacement of explicit ruff::Expr:: variant names with Self:: throughout the expression matching and construction code improves readability and follows Rust conventions. This makes the code more maintainable and consistent with the broader codebase refactoring.

Also applies to: 58-60, 62-64, 66-68, 70-72, 74-78, 80-82, 88-92, 94-96, 98-102, 104-106, 108-112, 114-118, 120-122, 124-126, 128-130, 147-149, 167-171

50-50: Well-organized code with improved readability.

The addition of blank lines after method implementations creates clear visual separation between different AST node implementations, improving code organization and readability.

Also applies to: 141-141, 157-157, 178-178, 198-198, 219-219, 242-242, 268-268, 304-304, 324-324, 345-345, 368-368, 394-394, 418-418, 445-445, 459-459, 474-474, 494-494, 515-515, 535-535, 556-556, 579-579, 605-605, 626-626, 649-649, 678-678, 692-692, 706-706, 720-720, 736-736, 763-763, 795-795, 826-826, 854-854, 877-877, 903-903, 951-951, 987-987, 1002-1002, 1019-1019, 1035-1035, 1056-1056, 1077-1077, 1099-1099, 1122-1122, 1142-1142, 1159-1159, 1180-1180, 1205-1205, 1236-1236

33-33: Good addition of associated type annotations.

Adding the type ConstantBag declarations improves trait clarity and explicitly documents the type relationships in the marshal implementations.

Also applies to: 130-130

135-135: Improved code organization with consistent spacing.

The addition of blank lines between method implementations enhances readability and creates clear visual separation between different functionality.

Also applies to: 139-139, 143-143, 147-147, 151-151, 155-155, 159-159, 163-163, 167-167, 172-172, 176-176, 183-183, 195-195, 203-203, 215-215

184-194: More explicit implementations improve code clarity.

The updated make_set and make_frozenset implementations are more explicit about their operations, making the code easier to understand while maintaining the same functionality.

Also applies to: 196-202

24-37: 👍 Const-ifying the numeric constructors looks good

new_int and new_float contain only field initialisation, so they are safely promotable to const fn.
No functional or compile-time issues spotted.

38-43: 👍 new_complex is also a safe const fn

Same rationale as above – just a plain struct literal.

52-71: 👍 Remaining trivial constructors are fine

new_bool, new_none, and new_ellipsis are straightforward and safe as const fn.

141-166: Enum shorthand improves readability

Switching to Self::Variant removes noise without altering logic.

255-257: Good use of const fn for the constructor

Making new a const function is appropriate here since it only performs field initialization.

1908-1914: Appropriate use of const fn for constant methods

Both methods return compile-time constants, making them ideal candidates for const functions.

2047-2058: Good refactoring to use Self

Using Self instead of the explicit type name improves maintainability and follows Rust best practices.

2219-2219: Consistent improvements with Self and const fn

Good use of Self in the constructor and appropriate const fn for the simple getter method.

Also applies to: 2260-2265

2737-2746: Well-implemented getter methods

The new getters follow the established pattern, properly acquiring locks and delegating or returning cloned values as appropriate.

2897-2939: Well-designed helper struct for string slicing

The SlicedStr struct is well-implemented with appropriate use of const fn and #[inline] attributes for performance optimization.

3493-3498: Consistent refactoring in StringIO

Good use of Self in the constructor and appropriate const fn for methods returning constants.

Also applies to: 3515-3527

3846-3857: Good const fn and Self usage

The rawmode method is ideal for const fn as it only performs pattern matching on compile-time values. The Self:: usage in the enum improves maintainability.

Also applies to: 3870-3879

4154-4160: Consistent implementation of writable() method

The new writable() method follows the same pattern as readable() and the Self:: usage improves code maintainability.

Also applies to: 4439-4442