Use reject instead of delete_if #592

jyaworski · 2016-04-12T20:53:50Z

@smoeding alerted me to this problem.

smoeding · 2016-04-13T06:37:36Z

I think this still includes a change that would break existing functionality.

stdlib 4.11:

delete([ 'abracadabra' ], 'bra') => [ 'abracadabra' ]
delete([ 'abracadabra' ], 'abr') => [ 'abracadabra' ]

With this proposed change:

delete([ 'abracadabra' ], 'bra') => [ 'abracadabra' ]
delete([ 'abracadabra' ], 'abr') => [ ]

Maybe a new optional parameter would be helpful, so the user could specify if the second parameter should be used literally or as a regular expression. Otherwise all special regex characters would lead to a semantic change anyway. What about this for example:

delete([ 'a', 'b', 'c', '.' ], '.')

jyaworski · 2016-04-13T14:29:42Z

I'm OK with adding a flag to turn on the regex matching, but I'm really confused why your second case is matching. Do we need to explicitly do something like ^expr$?

Any ideas on what that flag might look like? An entirely different function even?

smoeding · 2016-04-13T16:21:57Z

In your previous commit you added (coll_item =~ %r{#{item}}) == 0 as condition. The result of the match operator is the position of the match. Matching abr in abracadabra matches from position 0. So in this case every element that starts with the item is removed.

Using anchors would somehow solve this problem. But it would also force the user to provide a full matching regex. To remove all entries that contain the letter 'a' somewhere would have to be written as .*a.* while everywhere else (grep, awk, sed, perl, ruby, ...) a simple a would suffice. I think this would be confusing.

So one solution could be an additional method parameter (e.g. is_regex = false) that the user has to set to true to switch to regex matching. Without this parameter the simple string comparison just as before would be used.

jyaworski · 2016-04-13T18:26:32Z

Ah, OK. I couldn't find docs on how regex matching return codes worked. We want to do whole-word matching. \b, possibly, from this link. http://stackoverflow.com/questions/4879395/ruby-regex-rejecting-whole-words

jyaworski · 2016-04-20T05:32:25Z

OK @smoeding and @hunner: I think I've got this.

I did end up having to accept a third, optional argument. There's no other way (that I saw) to do this reasonably and preserve existing functionality without making a lot of assumptions (like whole word regex matching by default).

DavidS · 2016-04-20T14:20:50Z

spec/functions/delete_spec.rb

@@ -4,19 +4,22 @@
 it { is_expected.not_to eq(nil) }
 it { is_expected.to run.with_params().and_raise_error(Puppet::ParseError) }
 it { is_expected.to run.with_params([]).and_raise_error(Puppet::ParseError) }
- it { is_expected.to run.with_params([], 'two', 'three').and_raise_error(Puppet::ParseError) }
+ it { is_expected.to run.with_params([], 'two', 'three') }


this first block of tests test that the wrong number of arguments raises an error. as you're adding an optional third param, the correct fix here is

it { is_expected.to run.with_params([], 'two', 'three', 'four').and_raise_error(Puppet::ParseError) }

DavidS · 2016-04-20T14:25:09Z

Hi @jyaworski , a flag seems to be indeed the only way forward here. Good work overall, only a few minor nits around docs and tests.

jyaworski · 2016-04-20T14:49:18Z

@DavidS fixed. Thanks.

nibalizer · 2016-04-21T17:47:41Z

README.markdown

@@ -265,7 +265,7 @@ Takes a resource reference and an optional hash of attributes. Returns 'true' if

 #### `delete`

-Deletes all instances of a given element from an array, substring from a string, or key from a hash. Arrays and hashes may also match on regular expressions. For example, `delete(['a','b','c','b'], 'b')` returns ['a','c']; `delete('abracadabra', 'bra')` returns 'acada'. `delete({'a' => 1,'b' => 2,'c' => 3},['b','c'])` returns {'a'=> 1}, `delete(['abf', 'ab', 'ac'], '^ab.*')` returns ['ac']. *Type*: rvalue.
+Deletes all instances of a given element from an array, substring from a string, or key from a hash. Arrays and hashes may also match on regular expressions by setting an optional third parameter to a true value. For example, `delete(['a','b','c','b'], 'b')` returns ['a','c']; `delete('abracadabra', 'bra')` returns 'acada'. `delete({'a' => 1,'b' => 2,'c' => 3},['b','c'])` returns {'a'=> 1}, `delete(['abf', 'ab', 'ac'], '^ab.*', true)` returns ['ac']. `delete(['ab', 'b'], 'b')` returns ['ab']. *Type*: rvalue.


This line is really quite long, can it be broken up?

DavidS · 2016-04-21T18:01:31Z

We've been discussing this in Triage, and it turned out that this is a backwards incompatible:

it { is_expected.to run.with_params(['one', 'two', 'twentytwo', 'another two'], 'two').and_return(['one']) }

passes on the current implementation, but fails on yours, so that's not good, either.

jyaworski · 2016-04-21T18:14:27Z

@DavidS @hunner so it looks like we need another function entirely then. Please revert #591 and I'll change this PR to add a delete_regex function.

DavidS · 2016-04-21T18:20:48Z

Ohhh! so the "other" behaviour is not yet released? That then, of course, doesn't make this a breaking change. More investigation required.

bmjen · 2016-04-21T18:21:43Z

I don't think you need a delete_regex function, because the regex matching will work fine with this 1-line change: collection.reject! { |coll_item| (coll_item =~ %r{#{item}}) } here https://github.com/puppetlabs/puppetlabs-stdlib/pull/592/files#diff-85b753300e047711d62ec6a8a075a7deL35.

It looks like what you want is a delete function that will match on absolute string matching... based on the diff in functionality. This can be accomplished via a new function.. or the user providing a regex that of ^string$

jyaworski · 2016-04-22T04:18:45Z

To be clear, this is the functionality I'm trying to do:

delete(['lo', 'bond0', 'bond1', 'eth0'], '^bond.*$') => ['lo', 'eth0']

Delete all keys/entries in a hash/array matching a regular expression.

jyaworski · 2016-04-28T14:14:02Z

Okay @bmjen and @hunner. This is no longer backwards-incompatible.

bmjen · 2016-05-04T15:29:05Z

This looks good @jyaworski! Thanks for making the updates. Can you rebase this and we can push it through. 👍

jyaworski · 2016-05-04T15:33:39Z

@bmjen done. Thanks.

jyaworski mentioned this pull request Apr 12, 2016

daemon_options appears to be broken with the latest stdlib smoeding/puppet-sendmail#8

Closed

jyaworski changed the title ~~Use reject instead of delete_if, and check for == 0~~ Use reject instead of delete_if Apr 20, 2016

DavidS reviewed Apr 20, 2016
View reviewed changes

nibalizer reviewed Apr 21, 2016
View reviewed changes

bmjen added the backwards-incompatible label Apr 21, 2016

bmjen added needs-rebase and removed backwards-incompatible labels May 4, 2016

Use reject instead of delete_if

540546b

bmjen merged commit f46c9fd into puppetlabs:master May 4, 2016

david22swan added bugfix and removed needs-rebase labels Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use reject instead of delete_if #592

Use reject instead of delete_if #592

jyaworski commented Apr 12, 2016

smoeding commented Apr 13, 2016

jyaworski commented Apr 13, 2016

smoeding commented Apr 13, 2016

jyaworski commented Apr 13, 2016

jyaworski commented Apr 20, 2016

DavidS Apr 20, 2016

jyaworski Apr 21, 2016

DavidS commented Apr 20, 2016

jyaworski commented Apr 20, 2016

nibalizer Apr 21, 2016

jyaworski Apr 21, 2016

DavidS commented Apr 21, 2016

jyaworski commented Apr 21, 2016

DavidS commented Apr 21, 2016

bmjen commented Apr 21, 2016

jyaworski commented Apr 22, 2016

jyaworski commented Apr 28, 2016

bmjen commented May 4, 2016

jyaworski commented May 4, 2016

Use reject instead of delete_if #592

Use reject instead of delete_if #592

Conversation

jyaworski commented Apr 12, 2016

smoeding commented Apr 13, 2016

jyaworski commented Apr 13, 2016

smoeding commented Apr 13, 2016

jyaworski commented Apr 13, 2016

jyaworski commented Apr 20, 2016

DavidS Apr 20, 2016

Choose a reason for hiding this comment

jyaworski Apr 21, 2016

Choose a reason for hiding this comment

DavidS commented Apr 20, 2016

jyaworski commented Apr 20, 2016

nibalizer Apr 21, 2016

Choose a reason for hiding this comment

jyaworski Apr 21, 2016

Choose a reason for hiding this comment

DavidS commented Apr 21, 2016

jyaworski commented Apr 21, 2016

DavidS commented Apr 21, 2016

bmjen commented Apr 21, 2016

jyaworski commented Apr 22, 2016

jyaworski commented Apr 28, 2016

bmjen commented May 4, 2016

jyaworski commented May 4, 2016