Skip to content

bug # 20681 delete() function should not remove elements from original list #178

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

Merged
merged 1 commit into from
Sep 17, 2013

Conversation

lmello
Copy link
Contributor

@lmello lmello commented Sep 9, 2013

The setup: list with 3 elements, delete one:
$test_list = [‘a’, ‘b’, ‘c’]
$test_deleted = delete($test_list, ‘a’)

Print out the elements in ‘test_deleted’:
notify { ‘group_output2’: withpath => true, name => “$cfeng::test_deleted”, }
Notice: /Stage[main]/Syslog/Notify[group_output2]/message: bc

Good! Run-on output shows that ‘a’ was deleted

Print out the elements in ‘test_list’:
notify { ‘group_output1’: withpath => true, name => “$cfeng::test_list”, }
Notice: /Stage[main]/Syslog/Notify[group_output1]/message: bc

WHAT!? 'a' was deleted from ‘test_list’ as well! Expected abc as output!

This behaviour is confirmed for string, hash and array.
This is fixed on this commit, I had added two spec tests to cover that cases.

@lmello
Copy link
Contributor Author

lmello commented Sep 10, 2013

I had changed to using collection = arguments[0].dup instead of rejecting elements from the original list, as done on the previous commit.

This change on aproach is needed because the previous solution does not worked on ruby 1.8.7, (silly me, tested only on 1.9.3 and 2.0.0 ).

The current fix is more simple and works on ruby 1.8.7, 1.9.3 and 2.0.0.

item = arguments[1]

case collection
when Array, Hash
collection.delete item
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this whitespace change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it was a typo error. i will fix that.

@lmello
Copy link
Contributor Author

lmello commented Sep 12, 2013

I had identified that other delete functions also have this same issue. I will fix that tomorrow.
Best

@adrienthebo
Copy link
Contributor

@lmello are you planning on filing new pull requests for the other delete functions are you going to append commits to this pull request? If it's the former, I can merge this in.

@lmello
Copy link
Contributor Author

lmello commented Sep 14, 2013

On monday i will amend all those commits into a single one. Then you could merge if everything is ok.

I will fill new pull requests for the other functions.

best.

…l list

The setup: list with 3 elements, delete one:
$test_list = [‘a’, ‘b’, ‘c’]
$test_deleted = delete($test_list, ‘a’)

Print out the elements in ‘test_deleted’:
notify { ‘group_output2’:  withpath => true, name     => “$cfeng::test_deleted”, }
Notice: /Stage[main]/Syslog/Notify[group_output2]/message: bc

Good!  Run-on output shows that ‘a’ was deleted

Print out the elements in ‘test_list’:
notify { ‘group_output1’: withpath => true, name     => “$cfeng::test_list”, }
Notice: /Stage[main]/Syslog/Notify[group_output1]/message: bc

WHAT!?  'a' was deleted from ‘test_list’ as well! Expected abc as output!

This behaviour is confirmed for string, hash and array.
This is fixed on this commit, I had  added two spec tests to cover that cases.

bug #20681 spec test for delete() function.

I had forgot in the last commit the spec test for hash in the
delete function.

bug # 20681 delete() function change aproach.

Instead of rejecting elements from the original list, we use
collection = arguments[0].dup .
then latter we could continue to use delete and gsub! on collection
without impact on original argument.

this is a better solution than the previous one, and works on ruby
1.8.7, 1.9.3 and 2.0.0.

The previous solution does not work on ruby 1.8.7.

delete function remove typo whitespace.

fix typo whitespaces.
@lmello
Copy link
Contributor Author

lmello commented Sep 17, 2013

please merge. :-)

adrienthebo added a commit that referenced this pull request Sep 17, 2013
 bug # 20681 delete() function should not remove elements from original list
@adrienthebo adrienthebo merged commit 5cc5e29 into puppetlabs:master Sep 17, 2013
@adrienthebo
Copy link
Contributor

summary: merged into master in 5cc5e29; thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants