Skip to content

Commit bfe3787

Browse files
alexanderadamalexanderadam
committed
document UnreliableSubclasses in styleguide and changelog
include a 'good' example showing an explicit registry, plus pointers to eager loading and ActiveSupport::DescendantsTracker for cases where reflection is unavoidable.
1 parent 0d55d02 commit bfe3787

2 files changed

Lines changed: 45 additions & 0 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# rubocop-github
22

3+
## Unreleased
4+
5+
- Added `GitHub/UnreliableSubclasses` cop. Flags `Class#descendants` and `Class#subclasses` when the receiver is a constant. Both happily skip classes that haven't been autoloaded yet. Both also depend on GC timing for dynamically-defined classes, which is great fun in tests.
6+
37
## v0.26.0
48

59
- Read the automatic release notes on [the /releases page for this gem](https://github.com/github/rubocop-github/releases).

‎STYLEGUIDE.md‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ This is GitHub's Ruby Style Guide, inspired by [RuboCop's guide][rubocop-guide].
3131
18. [Rails](#rails)
3232
1. [content_for](#content_for)
3333
2. [Instance Variables in Views](#instance-variables-in-views)
34+
19. [Subclasses](#subclasses)
35+
1. [Avoid Class#descendants and Class#subclasses](#avoid-classdescendants-and-classsubclasses)
3436

3537
## Layout
3638

@@ -1096,4 +1098,43 @@ If you need to call a subview that expects an instance variable be set. If possi
10961098

10971099
Unfortunately the only way to get data into a layout template is with instance variables. You can't explicitly pass locals to them.
10981100

1101+
## Subclasses
1102+
1103+
### Avoid `Class#descendants` and `Class#subclasses`
1104+
1105+
Skip `Class#descendants` (ActiveSupport) and `Class#subclasses` (Ruby). They might lie to you in two ways:
1106+
1107+
* If a class hasn't been autoloaded yet, they don't see it. So your answer depends on what the app happened to touch first.
1108+
* GC can drop dynamically defined classes whenever it feels like it. Tests love this one.
1109+
1110+
If you really, really need it, add a `rubocop:disable` on the line with a short note so that future-you isn't confused.
1111+
1112+
* <a href="https://github.com/github/rubocop-github/blob/main/lib/rubocop/cop/github/unreliable_subclasses.rb">RuboCop rule: GitHub/UnreliableSubclasses</a>
1113+
1114+
``` ruby
1115+
class Person < ApplicationRecord
1116+
end
1117+
1118+
class Employee < Person
1119+
end
1120+
1121+
# bad
1122+
Person.descendants # => maybe [Employee], maybe [], who knows? Not me! I never lost control.
1123+
Person.subclasses # => same problem
1124+
1125+
# good. Keep an explicit registry
1126+
class Person < ApplicationRecord
1127+
TYPES = []
1128+
1129+
def self.inherited(subclass)
1130+
super
1131+
TYPES << subclass
1132+
end
1133+
end
1134+
1135+
Person::TYPES # => [Employee, ...]
1136+
```
1137+
1138+
Other alternatives: eager load the dependency tree ([`Rails.application.eager_load!`](https://api.rubyonrails.org/classes/Rails/Application.html#method-i-eager_load-21) in tests, `config.eager_load = true` in prod) or use [`ActiveSupport::DescendantsTracker`](https://api.rubyonrails.org/classes/ActiveSupport/DescendantsTracker.html) directly if you really need the reflection.
1139+
10991140
[rubocop-guide]: https://github.com/rubocop-hq/ruby-style-guide

0 commit comments

Comments
 (0)