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

Fix select #18

Merged
merged 9 commits into from
Mar 21, 2017
Merged

Fix select #18

merged 9 commits into from
Mar 21, 2017

Conversation

jreidinger
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.815% when pulling c7df963 on fix_select into 6661d65 on master.

@@ -217,7 +217,7 @@ def []=(key, value)
# @param matcher [Matcher]
# @return [Array<AugeasElement>] matching elements
def select(matcher)
@data.select(&matcher)
data.select(&matcher)
Copy link
Contributor

Choose a reason for hiding this comment

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

Before this fix, this line is one of the four lines in this file not covered by tests :-)

describe "#select" do
it "selects entries according to passed block" do
matcher = CFA::Matcher.new(collection: "spec")
expect(tree.select(matcher).size).to eq 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand this test at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, we have matcher, that select all elements in collection and then it verify that it find all 2 elements in collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and later we do same, but before we delete one of elements in collection

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, yes, this part I understand. I meant to say that it is very hard to verify that the test does the right thing, because you have to look up the fixture config file, and the lens file (which is located completely elsewhere), then understand the rather complex lens.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 88.008% when pulling 41d5ace on fix_select into 6661d65 on master.

cfa.gemspec Outdated
@@ -1,6 +1,6 @@
Gem::Specification.new do |s|
s.name = "cfa"
s.version = "0.5.1"
s.version = "0.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI cfa_grub2 will cry.

@@ -198,6 +198,23 @@ def run

attr_reader :aug

# Removes entry from tree. If path do not exist, then try if it don't
Copy link
Contributor

Choose a reason for hiding this comment

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

Grrrammar...

# "test" => remove, "lest" => remove, "test" => add
# in this case it change after first add
# "test[1]" => remove, "lest" => remove, "test[2]" => already added
# so in this case try to append [1] to path
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this means. Also, the verb grammar is bad.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 88.057% when pulling 93de23e on fix_select into 6661d65 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 88.057% when pulling eea99f3 on fix_select into 6661d65 on master.

@jreidinger jreidinger merged commit 540da5a into master Mar 21, 2017
@jreidinger jreidinger deleted the fix_select branch March 21, 2017 15:50
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