Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for custom method qualifiers #11

Merged
merged 1 commit into from
Aug 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ Name | Default value | Configurable values
--- | --- | ---
EnforcedStyle | `'alphabetical'` | `'alphabetical'`
IgnoredMethods | `['initialize']` | Array
MethodQualifiers | `[]` | Array
Signature | `nil` | `'sorbet'`, `nil`

#### Example
Expand All @@ -84,7 +85,10 @@ Signature | `nil` | `'sorbet'`, `nil`
# .rubocop.yml
Layout/OrderedMethods:
EnforcedStyle: alphabetical
IgnoredMethods: initialize
IgnoredMethods:
- initialize
MethodQualifiers:
- memoize
Signature: sorbet
```

Expand Down Expand Up @@ -136,6 +140,16 @@ protected :instance_a
public :instance_a
```

#### Method qualifiers
Some gems (like `memery`, `memoist`, etc.) provide a DSL that modifies the method (e.g. for memoization).
Those DSL methods can be added to the `MethodQualifiers` configuration, and they will be respected.

E.g. the following source can be correctly ordered:
```ruby
def b; end;
memoize def a;end
```

#### Method signatures

Support for (Sorbet) method signatures was added to the corrector by
Expand Down
26 changes: 20 additions & 6 deletions lib/rubocop/cop/layout/ordered_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ class OrderedMethods < Cop
include RangeHelp

COMPARISONS = {
'alphabetical' => lambda do |left_method, right_method|
(left_method.method_name <=> right_method.method_name) != 1
'alphabetical' => lambda do |left_node, right_node|
(method_name(left_node) <=> method_name(right_node)) != 1
end
}.freeze
ERR_INVALID_COMPARISON = 'Invalid "Comparison" config for ' \
Expand Down Expand Up @@ -104,10 +104,7 @@ def consecutive_methods(nodes)

def filter_relevant_nodes(nodes)
nodes.select do |node|
(
(node.defs_type? || node.def_type?) &&
!ignored_method?(node.method_name)
) || (node.send_type? && node.bare_access_modifier?)
relevant_node?(node) || (node.send_type? && qualifier_macro?(node))
end
end

Expand All @@ -133,12 +130,29 @@ def group_methods_by_access_modifier(nodes)
end
end

def self.method_name(node)
return node.method_name unless node.send_type?

node.first_argument.method_name
end

def ordered?(left_method, right_method)
comparison = COMPARISONS[cop_config['EnforcedStyle']]
raise Error, ERR_INVALID_COMPARISON if comparison.nil?

comparison.call(left_method, right_method)
end

def relevant_node?(node)
(node.defs_type? || node.def_type?) && !ignored_method?(node.method_name)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to use the extracted .method_name to get the node's method name here and on line 153?

Copy link
Contributor Author

@Darhazer Darhazer Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really necessary, since it first check that the node is either def or defs, and the method_name handles send nodes, in order to get the defined method name, and not the name of the macro (e.g. get foo and not memoize in case of memoize def foo.
Though since it will return the same (node.method_name) for a node that is not of a send type, I can call it. I'd rather keep it as is, as method_name is defined on the class level

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Thanks!

end

def qualifier_macro?(node)
return true if node.bare_access_modifier?

cop_config['MethodQualifiers'].to_a.include?(node.method_name.to_s) &&
relevant_node?(node.first_argument)
end
end
end
end
Expand Down
11 changes: 9 additions & 2 deletions lib/rubocop/cop/qualifier_node_matchers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,16 @@ module QualifierNodeMatchers
def_node_matcher :alias_method?,
'(send nil? {:alias_method} ... (sym $_method_name))'
def_node_matcher :qualifier?, <<-PATTERN
(send nil? {#{QUALIFIERS.map(&:inspect).join(' ')}}
... (sym $_method_name))
(send nil? #method_qualifier? ... (sym $_method_name))
PATTERN

def method_qualifier?(name)
qualifiers.include?(name)
end

def qualifiers
@qualifiers ||= QUALIFIERS + @cop_config['MethodQualifiers'].to_a.map(&:to_sym)
end
end
end
end
40 changes: 40 additions & 0 deletions spec/rubocop/cop/layout/ordered_methods_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,46 @@ def b; end
end
end

context 'with configured method qualiifers' do
let(:cop_config) { {'MethodQualifiers' => %w[memoize]} }

it 'recognizes the qualifier as a class method as well' do
expect_offense(<<~RUBY)
class Foo
def b; end
def a; end
^^^^^^^^^^ Methods should be sorted in alphabetical order.
memoize :a
end
RUBY

expect_correction(<<~RUBY)
class Foo
def a; end
memoize :a
def b; end
end
RUBY
end

it 'registers an offense when methods are not in alphabetical order' do
expect_offense(<<~RUBY)
class Foo
def b; end
memoize def a; end
^^^^^^^^^^^^^^^^^^ Methods should be sorted in alphabetical order.
end
RUBY

expect_correction(<<~RUBY)
class Foo
memoize def a; end
def b; end
end
RUBY
end
end

# We integration-test our cop via `::RuboCop::CLI`. This is quite close to an
# end-to-end test, with the normal pros and cons that entails. We exercise
# more of our code, but our assertions are more fragile, for example asserting
Expand Down