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

Add a method for using within with an already existing section #178

Closed

Conversation

cwitthaus
Copy link

We use a lot of sections in our site_prism code, and I was trying to use the within functionality to scope some expectations to a specific row in a table. Unfortunately, since the sections are initialized and placed into an array when using something like page.rows(text: "value").first it's not possible to add a block and execute code within the scope of the section. This change should leave the original behavior in place, and also add a method that allows users to scope a block for a section created by a sections declaration.

@@ -12,7 +12,7 @@ class Section
def initialize(parent, root_element)
@parent = parent
@root_element = root_element
Capybara.within(@root_element) { yield(self) } if block_given?
within { yield(self) } if block_given?
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds an additional/unnecessary extra proc around the block execution when it gets passed to within. You should modify the initializer method here to specify the block in the parameter list, and pass the block to within as &block, which then will yield within the within method.

def initialize(parent, root_element, &block)
  # ..
  within(&block) if block_given?
end

SitePrism::Section.new(a_page, 'div') { 1 + 1 }
it 'passes a given block to #within' do
block_variable = 5
expect_any_instance_of(SitePrism::Section).to receive(:within).and_yield
Copy link
Contributor

Choose a reason for hiding this comment

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

use of expect_any_instance is discouraged. Also, this is stubbing the subject of the test.

The original expectation on Capybara should probably be kept but updated to use and_yield as you are doing here.

@@ -85,5 +87,23 @@ class Page < SitePrism::Page
expect(section).to respond_to :execute_script
expect(section).to respond_to :evaluate_script
end

it 'responds to within method' do
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is superfluous.


describe '#within' do
it 'passes a given block to Capybara.within' do
expect(Capybara).to receive(:within).with('locator')
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is duplicated by the next one and should be deleted.

@tmertens
Copy link
Contributor

Are you working on cleaning up these rubocop errors in another branch, or just ignoring them for now?

Likely they are from default settings in new versions of rubocop

@luke-hill
Copy link
Collaborator

Hi there, thanks for your PR. Sorry it's taken a while to get around to looking at it. I've only recently come onboard here.

As it's a small PR. It's likely we can try get this in a 2.10 or 2.11 release. I'm not sure which one yet, as there's a lot of tidying up I want to do first.

If possible can you provide me with a small code example showing me where you want to use this going forwards. As I see it this is simply going to provide you with a was to pre-filter on initialization. But I'd rather see a working example if at all possible.

@luke-hill
Copy link
Collaborator

Hi @cwitthaus If you're still floating around could you see if you can give us an example of the problem you're trying to solve with this PR. Just so we can triage and merge in ASAP.

Is there anything stopping you rescoping the sections once defined? Something along the lines of

def scoped_sections
  your_plural_sections.select { |section| section.boolean? }
end

@luke-hill
Copy link
Collaborator

Ping @cwitthaus

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.

None yet

3 participants