Skip to content

Conversation

viralpraxis
Copy link
Contributor

The documentation states that sign? returns boolean:

# Checks whether this is literal has a sign.
#
# @example
#
#   +42
#
# @return [Boolean] whether this literal has a sign.
def sign?
  source.match(SIGN_REGEX)
end

but it does not.

Looks like the only usage [1] of sign? is this [2] module, so nothing should break.

[1] https://github.com/search?q=org%3Arubocop%20sign%3F&type=code
[2] https://github.com/rubocop/rubocop/blob/ddbb2a1bb65a29ac2d2d963196f7b00821779fd6/lib/rubocop/cop/style/method_call_with_args_parentheses/omit_parentheses.rb#L226

@viralpraxis viralpraxis force-pushed the fix-rubocop-ast-numeric-node-sign-to-return-boolean branch from f6fb9c5 to ea78ca9 Compare May 4, 2025 18:22
@viralpraxis viralpraxis changed the title Fix RuboCop::AST::NumericNode to return boolean Fix RuboCop::AST::NumericNode#sign? to return boolean May 4, 2025
@viralpraxis viralpraxis force-pushed the fix-rubocop-ast-numeric-node-sign-to-return-boolean branch from ea78ca9 to 8b2e4f9 Compare May 4, 2025 20:47
@koic
Copy link
Member

koic commented May 4, 2025

Can you update specs for #sign? as shown below?

diff --git a/spec/rubocop/ast/rational_node_spec.rb b/spec/rubocop/ast/rational_node_spec.rb
index 8e27da8..a253ce3 100644
--- a/spec/rubocop/ast/rational_node_spec.rb
+++ b/spec/rubocop/ast/rational_node_spec.rb
@@ -10,16 +10,18 @@ RSpec.describe RuboCop::AST::RationalNode do
   end

   describe '#sign?' do
+    subject { rational_node.sign? }
+
     context 'when explicit positive rational' do
       let(:source) { '+0.2r' }

-      it { is_expected.to be_sign }
+      it { is_expected.to be(true) }
     end

     context 'when explicit negative rational' do
       let(:source) { '-0.2r' }

-      it { is_expected.to be_sign }
+      it { is_expected.to be(true) }
     end
   end

int_spec.rb and float_spec.rb likely need the same treatment as well.

The documentation states that `sign?` returns boolean:

```ruby
def sign?
  source.match(SIGN_REGEX)
end
```

but it does not.

Looks like the only usage [1] of `sign?` is this [2] module, so nothing should break.

[1] https://github.com/search?q=org%3Arubocop%20sign%3F&type=code
[2] https://github.com/rubocop/rubocop/blob/ddbb2a1bb65a29ac2d2d963196f7b00821779fd6/lib/rubocop/cop/style/method_call_with_args_parentheses/omit_parentheses.rb#L226

Co-authored-by: Koichi ITO <[email protected]>
@viralpraxis viralpraxis force-pushed the fix-rubocop-ast-numeric-node-sign-to-return-boolean branch from 8b2e4f9 to dd235ed Compare May 4, 2025 21:07
@viralpraxis
Copy link
Contributor Author

Can you update specs for #sign? as shown below?

diff --git a/spec/rubocop/ast/rational_node_spec.rb b/spec/rubocop/ast/rational_node_spec.rb
index 8e27da8..a253ce3 100644
--- a/spec/rubocop/ast/rational_node_spec.rb
+++ b/spec/rubocop/ast/rational_node_spec.rb
@@ -10,16 +10,18 @@ RSpec.describe RuboCop::AST::RationalNode do
   end

   describe '#sign?' do
+    subject { rational_node.sign? }
+
     context 'when explicit positive rational' do
       let(:source) { '+0.2r' }

-      it { is_expected.to be_sign }
+      it { is_expected.to be(true) }
     end

     context 'when explicit negative rational' do
       let(:source) { '-0.2r' }

-      it { is_expected.to be_sign }
+      it { is_expected.to be(true) }
     end
   end

int_spec.rb and float_spec.rb likely need the same treatment as well.

Sure. Note that either this one or https://github.com/rubocop/rubocop-ast/pull/379/files will have to be updated, depending on the merging order.

@koic
Copy link
Member

koic commented May 4, 2025

Yeah, I'm aware that #379's specs need to be addressed after this PR is merged. Thank you.

@viralpraxis
Copy link
Contributor Author

Sorry for ping @koic, but can we proceed with this one?

@marcandre marcandre merged commit c8a2a52 into rubocop:master Jul 16, 2025
22 checks passed
@marcandre
Copy link
Contributor

Thanks @viralpraxis. I have a branch that I need to finish and merge that fixes all of the predicates, including matchers... But this can be merged now. Do you need a release for this?

@koic
Copy link
Member

koic commented Jul 16, 2025

@marcandre It would be preferable if #379 is included in the next release.

@marcandre
Copy link
Contributor

Thanks @koic . Released as v1.46.0

Please don't hesitate to ping me after a few days, I sometimes miss notifications 😅

@viralpraxis viralpraxis deleted the fix-rubocop-ast-numeric-node-sign-to-return-boolean branch July 16, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants