Fix warning string literal #241
Conversation
WalkthroughThe update refactors the logic for appending parts of DSL commands extracted from HTML comments in ERB files. It introduces explicit boolean variables to determine whether to insert a newline or space between parts, enhancing clarity and control in the string concatenation process. No public interfaces were altered. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/deface/dsl/loader.rb (3)
82-83: Coerce the regex result to a proper boolean for clarity.
=~returns the match index (anInteger) ornil; in Ruby any integer—including0—is truthy. Using the raw return value makes the intent less explicit and can bite future refactors. Wrap the expression in a double-negation to signal that we really want a boolean:-ends_with_quote = dsl_commands =~ /('|")\z/ +ends_with_quote = !!(dsl_commands =~ /['"]\z/) - -starts_with_non_data_char = part =~ /\A[^\d:='"%]/ +starts_with_non_data_char = !!(part =~ /\A[^\d:='"%]/)
85-85: Add parentheses to make the ternary’s condition obvious.Because
||has higher precedence than?:, the current line is parsed as
(ends_with_quote || starts_with_non_data_char) ? "\n" : ' ', but this is not self-evident on first read. A pair of parentheses removes any doubt and silences linters that flag the ambiguity:- divider = ends_with_quote || starts_with_non_data_char ? "\n" : ' ' + divider = (ends_with_quote || starts_with_non_data_char) ? "\n" : ' '
85-87: Avoid the leading divider & reduce allocations.On the very first token
dsl_commandsis empty, so we prepend"\n"even though no
separator is needed, resulting in DSL strings that start with a blank line.
Additionally, building an array and callingjoineach iteration creates two
unnecessary objects. Using<<is both simpler and more memory-friendly:- divider = (ends_with_quote || starts_with_non_data_char) ? "\n" : ' ' - dsl_commands = [dsl_commands, divider, part].join('') + unless dsl_commands.empty? + dsl_commands << ((ends_with_quote || starts_with_non_data_char) ? "\n" : ' ') + end + dsl_commands << partThis removes the spurious initial newline and cuts per-iteration allocations to
a single append.
After Ruby 3.4 upgrade, there is a new warning for `frozen string literal`. Refactor DSL command extraction to improve comment handling and formatting
DanielePalombo
force-pushed
the
fix-literal-string-deface
branch
from
13e486e to
4b6617f
Compare
2 participants
ref #240
After Ruby 3.4 upgrade, there is a new warning for
frozen string literal. Refactor DSL command extraction to improve comment handling and formattingSummary by CodeRabbit
Summary by CodeRabbit