-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
Add print page command to ruby binding #8920
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
Conversation
fcb3d59
to
6cabffb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few tips/tricks/fixes
def print_page(**options) | ||
options[:page_ranges] &&= Array(options[:page_ranges]) | ||
|
||
bridge.print_page(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the bridge here isn't defined as an accessor / reader to try mask it better. So you need to directly call the iVar @bridge
here.
def print_page(**options) | ||
options[:page_ranges] &&= Array(options[:page_ranges]) | ||
|
||
bridge.print_page(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. and all others.
|
||
module Selenium | ||
module WebDriver | ||
MAGIC_NUMBER = 'JVBER' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you set this as a let it's probably syntatically better.
let(:magic_number) { 'JVBER' }
end | ||
|
||
it 'should return base64 for print command' do | ||
expect(@driver.print_page().include?(MAGIC_NUMBER)).to be true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be re-written to expect(@driver.print_page).to include(magic_number)
Reason: Rspec includes most predicates built into ruby as rspec matchers
Use this style for the ones below
6cabffb
to
5d3f892
Compare
@luke-hill Applied your review comments. Please take a look. Thanks 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. There are also a few stylistic ruby specific things you could do. But I'll let you learn those yourself.
If inside /rb
you run the command rubocop
it will flag up minor tweaks. If you then run rubocop -A
or maybe rubocop -a
dependent on version it will auto fix many of them.
18fba7e
to
36c72e0
Compare
Kudos, SonarCloud Quality Gate passed! |
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Motivation and Context
Types of changes
Checklist