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

Backported code from the current master #543

Merged
merged 6 commits into from
Mar 9, 2017
Merged

Conversation

mchf
Copy link
Member

@mchf mchf commented Mar 9, 2017

#
# @param devregex [String] regex to filter by
# @return [Array] of ifcfg names
def get_devices(devregex = "[~]")
Copy link
Member

Choose a reason for hiding this comment

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

why parameter is not regexp and instead only string that is later interpreted as regexp?

Copy link
Member

Choose a reason for hiding this comment

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

also why regexp is not just ~ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, ask @teclator ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can improve it 👍 . It was one of my first cleanups and just use same match that was used with Builtins.regexpmatch

https://github.com/yast/yast-yast2/pull/433/files#diff-36f13a6d2bafc7ad7bcfa6f8a452e81dL704

PR: #433

Copy link
Member Author

Choose a reason for hiding this comment

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

@teclator I know ;-) I'll do so for master not maintenance branch.

expect(subject.get_devices("1")).not_to include "em1", "eth1", "br1"
end

it "filters with <[~]> by default" do
Copy link
Member

Choose a reason for hiding this comment

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

maybe i woulld name it filters devices with "~" by default

expect(subject.get_devices(".")).to eql []
end

it "returns all devices filtering with <''>" do
Copy link
Member

Choose a reason for hiding this comment

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

with empty string

expect(subject.get_devices("")).to eql devices
end

it "does not crash with exception" do
Copy link
Member

Choose a reason for hiding this comment

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

quite strong statement and you are never sure that it do not return expection. E.g. low memory or signal can get almost in every line of code.

expect { subject.get_devices }.not_to raise_error
end

it "doesn't carry empty strings" do
Copy link
Member

Choose a reason for hiding this comment

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

filters out empty strings

@mchf
Copy link
Member Author

mchf commented Mar 9, 2017

@jreidinger Are you fine with creating a PR with modifications based on your comments against master? This should be pure backport of code which already (more less) is in master.

@jreidinger
Copy link
Member

@mchf yes, I am fine.

@teclator
Copy link
Contributor

teclator commented Mar 9, 2017

@mchf @jreidinger, I'm fine with just merge this PR with the backported code but the improvements or modifications maybe would be great to do directly in SP1 and then merge into SP2 and master

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 58.697% when pulling 55e0e6c on mchf:backported into b807edb on yast:SLE-12-SP1.

.travis.yml Outdated
@@ -5,7 +5,7 @@ before_install:
# disable rvm, use system Ruby
- rvm reset
- wget https://raw.githubusercontent.com/yast/yast-devtools/master/travis-tools/travis_setup.sh
- sh ./travis_setup.sh -p "rake yast2-core yast2-devtools yast2-testsuite yast2-ruby-bindings yast2 yast2-pkg-bindings" -g "rspec:3.3.0 yast-rake gettext simplecov coveralls rubocop:0.29.1"
- sh ./travis_setup.sh -p "rake yast2-core yast2-devtools yast2-testsuite yast2-ruby-bindings yast2 yast2-pkg-bindings" -g "rspec:3.3.0 yast-rake gettext simplecov:0.10.0 coveralls rubocop:0.29.1"
Copy link
Member

Choose a reason for hiding this comment

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

better fix is to completelly remove simplecov and it pick right version

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 58.697% when pulling c15998c on mchf:backported into b807edb on yast:SLE-12-SP1.

Copy link
Member

@jreidinger jreidinger left a comment

Choose a reason for hiding this comment

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

LGTM

@mchf mchf merged commit 8dfd1fc into yast:SLE-12-SP1 Mar 9, 2017
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.

4 participants