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

Add delete_values() and delete_undef_values() functions #166

Merged
merged 1 commit into from Aug 5, 2013

Conversation

ptomulik
Copy link
Contributor

Hi,
delete_values() complements delete() for hashes - deletion is based on hash values, not keys,
delete_undef_values() may be used to filter-out "unset" parameters from hash. It then plays nicely with ensure_resource() or create_resources() - see motivating example in README.markdown. I leave this pull request if you consider this function useful enough. Tell me if i've reinvented the wheel.

Regards.

@adrienthebo
Copy link
Contributor

Thank you very much for this contribution! However I have some concerns about how widely used this would be. As a general rule of thumb functions added to stdlib should be usable by 75% of the users of the module, and I'm not sure this meets that criteria. It definitely has merit but I think this function would be better suited as a module. How does this sound to you?

@ptomulik
Copy link
Contributor Author

ptomulik commented Aug 1, 2013

I wrote it for apachex module I'm currently developing and thought I could leave this for others in a place where it could persist for longer time. Thought a while about sending feature request but decided to take short cuts and just put the code here. I know, it may be too specific for a general purpose module such as puppetlabs-stdlib in its current form. What if I put few minutes and change this delete_undef_values to delete_values such that the function will delete from Hash/Array all items with given value (so it could serve also my purpose of deleting undefs)? You already have the delete() function in puppetlabs-stdlib, which can handle Hashes (deletion based on keys), and delete_values could complement it in some way?

Update: do you have a tool/method to measure what would be possible use of new functions? I've seen some measures provided in other pull requests (#161) and wonder how were they obtained? Regarding these functions in separate module - of course I can do this, but I'm just one person with no company/foundation behind me and this code would probably disappear in short time or left unmaintained.

@ptomulik
Copy link
Contributor Author

ptomulik commented Aug 1, 2013

Added delete_values() - it just complements delete() for Hash type. It appears that I can't handle properly undef's in delete_values(), because undef is converted to empty String when passed as argument to function (#13210, #15329). I leave both functions here. If you think this stuff is of no use, just close this pull request :).

@adrienthebo
Copy link
Contributor

Ah yes, there is the corner case where :undef is magically turned into an empty string. :( Okay, you've convinced me of the necessity of this. I'm going to go through and add some comments on some parts of the implementation that caught my eye.


*Examples:*

delete({'a'=>'A','b'=>'B','c'=>'C','B'=>'D'}, 'B')
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean delete_values here?

@ptomulik
Copy link
Contributor Author

ptomulik commented Aug 2, 2013

Corrected.

@adrienthebo
Copy link
Contributor

This looks like it's about ready to merge, but before I do that would you be willing to squash these changes down to a single commit?

@ptomulik
Copy link
Contributor Author

ptomulik commented Aug 5, 2013

No problem, done.

@adrienthebo
Copy link
Contributor

Did you rebase all of the changes, or just the most recent changes?

@ptomulik
Copy link
Contributor Author

ptomulik commented Aug 5, 2013

All.

@adrienthebo
Copy link
Contributor

There we go, when I commented I didn't see that - perhaps github hadn't updated.

adrienthebo added a commit that referenced this pull request Aug 5, 2013
Add delete_values() and delete_undef_values() functions
@adrienthebo adrienthebo merged commit 5544be9 into puppetlabs:master Aug 5, 2013
@adrienthebo
Copy link
Contributor

summary: merged into master in 5544be9, thanks for the contribution!

@ptomulik ptomulik deleted the delete_undef_values branch August 8, 2013 14:42
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.

None yet

3 participants