Skip to content

Commit 855c211

Browse files
committed
Improve reliability and aesthetics of comments stripper
** First motivation Reliably remove comments from Rails config files when generating Suspenders apps. With the parser gem, we're now removing comments by processing their surrounding code using AST (abstract syntax tree) processors. We were using a regex to do that, which is text-based and thereby subject to edge cases that only a proper AST solution can handle. [This pull request](#1044) made changes to some config files to fix the Action Mailer asset host. While working on merging that PR, the tests were not passing because they looked for an output that was missing in one of the changed config files: config.action_mailer.asset_host = %q{"https://#{ENV\.fetch\("ASSET_HOST", ENV\.fetch\("APPLICATION_HOST"\)\)}} The PR code changes are kind of straightforward and that config option should have been present in the config file but it was not. The reason was that Suspenders was removing code comments with an overly permissive regex : (`/^.*#.*$/`), which removed every line containing a `#` character, no matter where they were in the line - so it was also removing the new Action Mailer config setting. That regex was also silently messing up with other config lines, for example: config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } As a quick fix to merge that PR, I've changed the regex to `/^\s*#.*$/` but it was not an ideal solution - which is what I'm striving for with this PR. With the old text-based regex solution, there were still nasty edge cases to deal with, and we may never know when they'd occur. Also, the previous regex did not work with inline comments. ** Second motivation The final aesthetics of the config files could be improved in my opinion. This was how they looked like: Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.active_storage.service = :local config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end end And with the newest solution they look like this: Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end end Which preserves newlines where appropriate, so that's easier to read. ** Third motivation This pull request uses the `parser` gem, and I feel like introducing that gem in the project may be a good move over the long term. For example, we need to finish up the [gem action](#1066) to sort `gem` declarations alphabetically when adding gems to the `Gemfile`, and the most reliable way to do that is with AST parsing. ** Other related changes 1. Make the indentation of some templates match the Rails environment's config files'. This goes along with the aesthetics introduced in this PR. 2. Because of (1), I've also found a bug that was concatenaning two lines of codes in `config/environments/production.rb` with no blank line between them (in email_generator.rb), so I'm taking the opportunity to fix it 3. standardrb fixes after updating to the latest standardrb version
1 parent 31ff7bc commit 855c211

File tree

14 files changed

+783
-25
lines changed

14 files changed

+783
-25
lines changed

.ruby-version

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2.7.2
1+
2.7.4

.standard.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
11
ignore:
22
- "templates/partials/db_optimizations_configuration.rb"
3+
- "templates/partials/pull_requests_config.rb"
4+
- "templates/partials/email_smtp.rb"

lib/suspenders.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,5 +27,6 @@
2727
require "suspenders/generators/production/single_redirect"
2828
require "suspenders/generators/staging/pull_requests_generator"
2929
require "suspenders/actions"
30+
require "suspenders/actions/strip_comments_action"
3031
require "suspenders/adapters/heroku"
3132
require "suspenders/app_builder"

lib/suspenders/actions.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ def replace_in_file(relative_path, find, replace)
66
unless contents.gsub!(find, replace)
77
raise "#{find.inspect} not found in #{relative_path}"
88
end
9-
File.open(path, "w") { |file| file.write(contents) }
9+
File.write(path, contents)
1010
end
1111

1212
def action_mailer_host(rails_env, host)
Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
require "parser/current"
2+
3+
module Suspenders
4+
module Actions
5+
class StripCommentsAction
6+
class << self
7+
def call(source)
8+
parser = Parser::CurrentRuby.new
9+
10+
source
11+
.then { |s| strip_comments(s, parser) }
12+
.then { |s| strip_trailing_whitespace(s) }
13+
.then { |s| strip_dup_newlines(s) }
14+
.then { |s| strip_leading_scope_newlines(s, parser) }
15+
end
16+
17+
private
18+
19+
def strip_comments(source, parser)
20+
StripComments.call(source, parser.reset)
21+
end
22+
23+
def strip_trailing_whitespace(source)
24+
source.gsub(/[[:blank:]]+$/, "")
25+
end
26+
27+
def strip_dup_newlines(source)
28+
source.gsub(/\n{2,}/, "\n\n")
29+
end
30+
31+
def strip_leading_scope_newlines(source, parser)
32+
StripLeadingScopeNewlines.call(source, parser.reset)
33+
end
34+
end
35+
36+
# Strips full-line and inline comments from a buffer but does
37+
# not remove whitespaces or newlines after the fact. Example
38+
# input:
39+
#
40+
# MyGem.application.configure do |config|
41+
# # Full-line comment
42+
# config.option1 = :value # Inline comment
43+
# end
44+
#
45+
# The output is:
46+
#
47+
# MyGem.application.configure do |config|
48+
#
49+
# config.option1 = :value
50+
# end
51+
class StripComments
52+
class << self
53+
def call(source, parser)
54+
buffer = Parser::Source::Buffer.new(nil, source: source)
55+
rewriter = Parser::Source::TreeRewriter.new(buffer)
56+
57+
_, comments = parser.parse_with_comments(buffer)
58+
59+
comments.each do |comment|
60+
strip_comment(comment, buffer, rewriter)
61+
end
62+
63+
rewriter.process
64+
end
65+
66+
private
67+
68+
def strip_comment(comment, buffer, rewriter)
69+
expr = comment.location.expression
70+
71+
if full_line_comment?(expr)
72+
expr = full_line_comment_expr(expr, buffer)
73+
end
74+
75+
rewriter.remove(expr)
76+
end
77+
78+
def full_line_comment_expr(expr, buffer)
79+
pos = BackwardStringScanner.beginning_of_line_pos(expr, expr.begin_pos)
80+
81+
Parser::Source::Range.new(buffer, pos, expr.end_pos + 1)
82+
end
83+
84+
def full_line_comment?(expr)
85+
expr.source == expr.source_line.lstrip
86+
end
87+
end
88+
end
89+
90+
# A tiny, non-stateful backward string scanner somewhat inspired
91+
# by Ruby's StringScanner. Ruby's StringScanner is unable to
92+
# seek backward on a string.
93+
class BackwardStringScanner
94+
def self.beginning_of_line_pos(expr, initial_pos)
95+
skip_before(expr, initial_pos) { |char| char == "\n" }
96+
end
97+
98+
def self.skip_before(expr, initial_pos, &block)
99+
skip_until(expr, initial_pos, -1, &block)
100+
end
101+
102+
def self.skip_until(expr, initial_pos, lookup_inc = 0)
103+
pos = initial_pos
104+
105+
loop do
106+
break if pos.zero?
107+
char = expr.source_buffer.source[pos + lookup_inc]
108+
break if yield(char)
109+
pos -= 1
110+
end
111+
112+
pos
113+
end
114+
end
115+
116+
# The intent of this class is purely aesthetic: remove leading
117+
# newlines inside of code scopes like blocks and begin/end.
118+
# Example input:
119+
#
120+
# module MyGem
121+
#
122+
# MyGem.application.configure do |config|
123+
#
124+
# config.option1 = true
125+
#
126+
# config.option2 = false
127+
# end
128+
# end
129+
#
130+
# The output is:
131+
#
132+
# module MyGem
133+
# MyGem.application.configure do |config|
134+
# config.option1 = true
135+
#
136+
# config.option2 = false
137+
# end
138+
# end
139+
class StripLeadingScopeNewlines
140+
def self.call(source, parser)
141+
buffer = Parser::Source::Buffer.new(nil, source: source)
142+
ast = parser.parse(buffer)
143+
144+
LeadingNewlineStripRewriter.new.rewrite(buffer, ast).lstrip
145+
end
146+
147+
class LeadingNewlineStripRewriter < Parser::TreeRewriter
148+
def on_module(node)
149+
strip_newline_before(node.children[1])
150+
strip_newline_after(node.children.last)
151+
152+
super
153+
end
154+
155+
def on_class(node)
156+
strip_newline_before(node.children[2])
157+
strip_newline_after(node.children.last)
158+
159+
super
160+
end
161+
162+
def on_begin(node)
163+
handle_begin(node)
164+
165+
super
166+
end
167+
168+
def on_kwbegin(node)
169+
strip_newline_before(node.children[0])
170+
strip_newline_after(node.children.last)
171+
172+
handle_begin(node)
173+
174+
super
175+
end
176+
177+
def on_block(node)
178+
strip_newline_before(node.children[2])
179+
strip_newline_after(node.children.last)
180+
181+
super
182+
end
183+
184+
private
185+
186+
def handle_begin(node)
187+
strip_blank_lines_between_setter_calls(node.children)
188+
189+
node.children.each do |child_node|
190+
send("on_#{child_node.type}", child_node)
191+
end
192+
end
193+
194+
def strip_blank_lines_between_setter_calls(children)
195+
pairs = children.each_cons(2).to_a
196+
197+
pairs.each do |(node_before, node_after)|
198+
if setter_call?(node_before) && setter_call?(node_after)
199+
strip_newline_before(node_after)
200+
end
201+
end
202+
end
203+
204+
def setter_call?(node)
205+
node.children[1].to_s.end_with?("=")
206+
end
207+
208+
def strip_newline_before(node)
209+
return if node.nil?
210+
211+
expr = node.location.expression
212+
end_pos = find_end_pos(expr, expr.begin_pos)
213+
begin_pos = find_begin_pos(expr, end_pos)
214+
215+
strip_source_range(expr, begin_pos, end_pos)
216+
end
217+
218+
def strip_newline_after(node)
219+
return if node.nil?
220+
221+
expr = node.location.expression
222+
source = expr.source_buffer.source
223+
224+
if source[expr.end_pos + 1] == "\n"
225+
strip_source_range(expr, expr.end_pos, expr.end_pos + 1)
226+
end
227+
end
228+
229+
def find_end_pos(expr, begin_pos)
230+
BackwardStringScanner.skip_until(expr, begin_pos) do |char|
231+
char == "\n"
232+
end
233+
end
234+
235+
def find_begin_pos(expr, end_pos)
236+
BackwardStringScanner.skip_before(expr, end_pos) do |char|
237+
char != "\n" && char != " "
238+
end
239+
end
240+
241+
def strip_source_range(expr, begin_pos, end_pos)
242+
remove(
243+
Parser::Source::Range.new(
244+
expr.source_buffer,
245+
begin_pos,
246+
end_pos
247+
)
248+
)
249+
end
250+
end
251+
end
252+
end
253+
end
254+
end

lib/suspenders/app_builder.rb

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -205,15 +205,10 @@ def remove_config_comment_lines
205205
]
206206

207207
config_files.each do |config_file|
208-
path = File.join(destination_root, "config/#{config_file}")
208+
path = Pathname(destination_root).join("config", config_file)
209+
source = Actions::StripCommentsAction.call(path.read)
209210

210-
accepted_content = File.readlines(path).reject { |line|
211-
line =~ /^\s*#.*$/ || line =~ /^$\n/
212-
}
213-
214-
File.open(path, "w") do |file|
215-
accepted_content.each { |line| file.puts line }
216-
end
211+
path.write(source)
217212
end
218213
end
219214

lib/suspenders/generators/production/email_generator.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@ def smtp_configuration
77
copy_file "smtp.rb", "config/smtp.rb"
88

99
prepend_file "config/environments/production.rb",
10-
%{require Rails.root.join("config/smtp")\n}
10+
%{require Rails.root.join("config/smtp")\n\n}
1111
end
1212

1313
def use_smtp
1414
inject_template_into_file(
1515
"config/environments/production.rb",
1616
"partials/email_smtp.rb",
17-
after: "config.action_mailer.perform_caching = false"
17+
after: "config.action_mailer.perform_caching = false\n"
1818
)
1919
end
2020

spec/support/file_operations.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
module FileOperations
22
module_function
33

4+
def read_file(file)
5+
TestPaths.app_path.join(file).read
6+
end
7+
48
def touch_file(file)
59
path = app_path.join(file)
610
path.join("..").mkpath

0 commit comments

Comments
 (0)