Skip to content

constant default_move_duration prevents appium to override it - #6189

Merged
titusfortner merged 1 commit into
SeleniumHQ:masterfrom
prakharrr:constant_fix
Jul 27, 2018
Merged

constant default_move_duration prevents appium to override it#6189
titusfortner merged 1 commit into
SeleniumHQ:masterfrom
prakharrr:constant_fix

Conversation

@prakharrr

@prakharrr prakharrr commented Jul 20, 2018

Copy link
Copy Markdown

@prakharrr
prakharrr force-pushed the constant_fix branch 2 times, most recently from f6c1283 to 6f1572f Compare July 20, 2018 22:14

@luke-hill luke-hill Jul 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you want to remove the constant, the best way of defining it will be as a private memoized method

private

def default_move_duration
  @default_move_duration ||= 0.25
end

You also can remove the reader and writer, as the writer isn't used and the reader becomes this memo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The reason for this PR is that the constant is being overridden in the Appium library, so this would change to specifically allow a writer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's fair, I still wouldn't have procedurally defined instance vars, but each to their own.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's no reason to wrap the default in a method. Leverage the accessor.

@titusfortner
titusfortner requested review from lmtierney and p0deje July 24, 2018 16:06
@p0deje

p0deje commented Jul 25, 2018

Copy link
Copy Markdown
Member

Looks good, but please fix unit tests and RuboCop:

./go //rb:unit-test
./go //rb:lint

@p0deje p0deje added the C-rb Ruby Bindings label Jul 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

since this is no longer a constant, it needs to be referenced as a method on the class that includes the PointerActions module. In this case, builder. Update this to be:

      let(:duration) { builder.default_move_duration }

…he spec which references the instance var as a method)
@prakharrr

Copy link
Copy Markdown
Author

lint and unit test works now.

@p0deje p0deje left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. Titus, please merge 👍

@titusfortner
titusfortner merged commit 76882be into SeleniumHQ:master Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants