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

Matcher delegate_method supports 'private: true' #1653

Merged

Conversation

pr0d1r2
Copy link
Contributor

@pr0d1r2 pr0d1r2 commented Oct 1, 2024

Allow to specify delegate_method using with_private - which is described in documentation:

class Account
  delegate :plan, to: :subscription, private: true
end

# RSpec
describe Account do
  it { should delegate_method(:plan).to(:subscription).with_private }
end

# Minitest
class PageTest < Minitest::Test
  should delegate_method(:plan).to(:subscription).with_private
end

@pr0d1r2 pr0d1r2 force-pushed the delegate_method-support-private-true branch from 1e34a9c to a0a2ab5 Compare October 1, 2024 06:41
Copy link
Member

@matsales28 matsales28 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@pr0d1r2 pr0d1r2 force-pushed the delegate_method-support-private-true branch from a0a2ab5 to dff710b Compare October 1, 2024 16:46
@pr0d1r2
Copy link
Contributor Author

pr0d1r2 commented Oct 1, 2024

@matsales28 Sweet! It is a pure pleasure to work with code specified that well with RSpec 🙏

@pr0d1r2
Copy link
Contributor Author

pr0d1r2 commented Oct 2, 2024

Found one edge-case that still needs coverage:

it { is_expected.to delegate_method(:videos).to(:current_user).with_prefix('user').with_private }

Generates error:

     NoMethodError:
       private method `user_videos' called for

@matsales28
Copy link
Member

Found one edge-case that still needs coverage:

it { is_expected.to delegate_method(:videos).to(:current_user).with_prefix('user').with_private }

Generates error:

     NoMethodError:
       private method `user_videos' called for

Yeah, it should also be good to test it in conjunction with the as qualifier.

  it { is_expected.to delegate_method(:videos).to(:current_user).with_prefix('user').as(:new_videos) }

Please let me know if you intend to add the coverage, we can wait for it to merge this PR, otherwise, I can work on improving the coverage, we'll likely need to change the code itself a bit as well to match the error message better.

@pr0d1r2 pr0d1r2 force-pushed the delegate_method-support-private-true branch from dff710b to 9c62dbb Compare October 3, 2024 06:55
@pr0d1r2
Copy link
Contributor Author

pr0d1r2 commented Oct 3, 2024

@matsales28 force-pushed two commits again. The first one specify all combinations. Waiting to see those error messages changes.

@matsales28 matsales28 merged commit 6f4de5a into thoughtbot:main Oct 8, 2024
21 checks passed
@matsales28
Copy link
Member

@pr0d1r2 Thanks again for the contribution, I just merged it!

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.

2 participants