Skip to content

Commit b2c6d62

Browse files
skipkayhilskipkayhil
committed
[rail_inspector] Fix linting defaults post-release
Previously, the linter would not validate that all versions with defaults were documented, just that documented versions were correct. This lead to no validation of defaults for a new version until the new version's header was added. Additionally, the header couldn't be added until the first default for that version was added because the linter would raise an error if a version header was present with no defaults. This commit fixes both of those problems. It now more strictly compares the _actual_ versioned defaults with the documented defaults so that it will never miss any based on which are currently documented. Additionally, it won't error if a header is present without any defaults (and due to the more strict comparison, this state is expected).
1 parent b0f4516 commit b2c6d62

File tree

5 files changed

+84
-72
lines changed

5 files changed

+84
-72
lines changed

‎tools/rail_inspector/lib/rail_inspector/configuring.rb

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,7 @@ def check
6767
Check::FrameworkDefaults.new(
6868
self,
6969
framework_defaults_by_version,
70-
doc
71-
.versioned_defaults
72-
.slice_before { |line| line.start_with?("####") }
73-
.to_a,
70+
doc.versioned_defaults,
7471
),
7572
Check::NewFrameworkDefaultsFile.new(
7673
self,

‎tools/rail_inspector/lib/rail_inspector/configuring/check/framework_defaults.rb

Lines changed: 36 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -15,61 +15,53 @@ def initialize(checker, defaults_by_version, documented_defaults)
1515
end
1616

1717
def check
18-
header, *defaults_by_version = @documented_defaults
18+
expected_text = +""
1919

20-
checker.doc.versioned_defaults =
21-
header +
22-
defaults_by_version
23-
.map { |defaults| check_defaults(defaults) }
24-
.flatten
25-
end
20+
expected_text =
21+
@defaults_by_version.reverse_each.map { |version, defaults|
22+
expected_text = +"#### Default Values for Target Version #{version}\n"
2623

27-
private
28-
def check_defaults(defaults)
29-
header, configs = defaults[0], defaults[2, defaults.length - 3]
30-
configs ||= []
24+
expected_text << "\n" unless defaults.empty?
3125

32-
version = header.match(/\d\.\d/)[0]
26+
defaults.map { |config, value|
27+
full_config =
28+
case config
29+
when /^[A-Z]/
30+
config
31+
when /^self/
32+
config.sub("self", "config")
33+
else
34+
"config.#{config}"
35+
end
3336

34-
generated_doc =
35-
@defaults_by_version[version]
36-
.map do |config, value|
37-
full_config =
38-
case config
39-
when /^[A-Z]/
40-
config
41-
when /^self/
42-
config.sub("self", "config")
43-
else
44-
"config.#{config}"
45-
end
37+
"- [`#{full_config}`](##{full_config.tr("._", "-").downcase}): `#{value}`\n"
38+
}.sort.each { |t| expected_text << t }
4639

47-
"- [`#{full_config}`](##{full_config.tr("._", "-").downcase}): `#{value}`"
48-
end
49-
.sort
40+
expected_text
41+
}.join("\n")
5042

51-
config_diff =
52-
Tempfile.create("expected") do |doc|
53-
doc << generated_doc.join("\n")
54-
doc.flush
43+
config_diff =
44+
Tempfile.create("expected") do |doc|
45+
doc << expected_text
46+
doc.flush
5547

56-
Tempfile.create("actual") do |code|
57-
code << configs.join("\n")
58-
code.flush
48+
Tempfile.create("actual") do |code|
49+
code << @documented_defaults
50+
code.flush
5951

60-
`git diff --color --no-index #{doc.path} #{code.path}`
61-
end
52+
`git diff --color --no-index #{doc.path} #{code.path}`
6253
end
54+
end
6355

64-
checker.errors << <<~MESSAGE unless config_diff.empty?
65-
#{checker.files.application_configuration}: Incorrect load_defaults docs
66-
--- Expected
67-
+++ Actual
68-
#{config_diff.split("\n")[5..].join("\n")}
69-
MESSAGE
56+
checker.errors << <<~MESSAGE unless config_diff.empty?
57+
#{checker.files.application_configuration}: Incorrect load_defaults docs
58+
--- Expected
59+
+++ Actual
60+
#{config_diff.split("\n")[5..].join("\n")}
61+
MESSAGE
7062

71-
[header, "", *generated_doc, ""]
72-
end
63+
checker.doc.versioned_defaults = expected_text
64+
end
7365
end
7466
end
7567
end

‎tools/rail_inspector/lib/rail_inspector/configuring/document.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,19 @@ class Configuring
55
class Document
66
class << self
77
def parse(text)
8-
before, versioned_defaults, general_config, after =
8+
before, *versioned_defaults, general_config, after =
99
text
1010
.split("\n")
1111
.slice_before do |line|
1212
[
13-
"### Versioned Default Values",
13+
"#### Default Values for Target Version",
1414
"### Rails General Configuration",
1515
"### Configuring Assets"
16-
].include?(line)
16+
].any? { |s| line.start_with?(s) }
1717
end
1818
.to_a
1919

20-
new(before, versioned_defaults, general_config, after)
20+
new(before, versioned_defaults.flatten.join("\n"), general_config, after)
2121
end
2222
end
2323

@@ -29,7 +29,7 @@ def initialize(before, versioned_defaults, general_config, after)
2929
end
3030

3131
def to_s
32-
(@before + @versioned_defaults + @general_config + @after).join("\n") +
32+
(@before + [@versioned_defaults] + @general_config + @after).join("\n") +
3333
"\n"
3434
end
3535
end

‎tools/rail_inspector/test/rail_inspector/configuring/check/framework_defaults_test.rb

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,36 @@ def test_identifies_self_when_file_uses_config
1616
}
1717
}
1818

19-
doc = [
20-
["### Versioned Default Values"],
21-
[
22-
"#### Default Values for Target Version 8.1",
23-
"",
24-
"- [`config.action_controller.escape_json_responses`](#config-action-controller-escape-json-responses): `false`",
25-
"- [`config.yjit`](#config-yjit): `!Rails.env.local?`",
26-
"",
27-
],
28-
[
29-
"#### Default Values for Target Version 8.0",
30-
"",
31-
"- [`Regexp.timeout`](#regexp-timeout): `1`",
32-
"- [`config.action_dispatch.strict_freshness`](#config-action-dispatch-strict-freshness): `true`",
33-
"",
34-
],
35-
]
36-
37-
check(defaults, doc).check
19+
check(defaults, <<~DOC).check
20+
#### Default Values for Target Version 8.1
21+
22+
- [`config.action_controller.escape_json_responses`](#config-action-controller-escape-json-responses): `false`
23+
- [`config.yjit`](#config-yjit): `!Rails.env.local?`
24+
25+
#### Default Values for Target Version 8.0
26+
27+
- [`Regexp.timeout`](#regexp-timeout): `1`
28+
- [`config.action_dispatch.strict_freshness`](#config-action-dispatch-strict-freshness): `true`
29+
DOC
30+
31+
assert_empty checker.errors
32+
end
33+
34+
def test_post_release
35+
defaults = {
36+
"8.0" => {
37+
"Regexp.timeout" => "1"
38+
},
39+
"8.1" => {},
40+
}
41+
42+
check(defaults, <<~DOC).check
43+
#### Default Values for Target Version 8.1
44+
45+
#### Default Values for Target Version 8.0
46+
47+
- [`Regexp.timeout`](#regexp-timeout): `1`
48+
DOC
3849

3950
assert_empty checker.errors
4051
end

‎tools/rail_inspector/test/rail_inspector/visitor/framework_default_test.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,18 @@ def test_inline_condition
113113
assert_equal("1", config["8.0"]["Regexp.timeout"])
114114
end
115115

116+
def test_post_release
117+
config = config_for_defaults <<~RUBY
118+
case target_version.to_s
119+
when "8.1"
120+
load_defaults "8.0"
121+
end
122+
RUBY
123+
124+
assert_includes config, "8.1"
125+
assert_equal({}, config["8.1"])
126+
end
127+
116128
private
117129
def wrapped_defaults(defaults)
118130
<<~RUBY

0 commit comments

Comments
 (0)