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

Added delete function for system(s). #5

Merged
merged 4 commits into from
Dec 2, 2012
Merged

Added delete function for system(s). #5

merged 4 commits into from
Dec 2, 2012

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Dec 1, 2012

Allows you to delete system profiles from RHN Satellite.

@duritong
Copy link
Owner

duritong commented Dec 2, 2012

Thanks for your contribution. In general this seems to be fine, but I would like to change 2 things:

  • Imho we should not change system_ids. With map! we are changing the passed object system_ids, which I don't like that much, because people might use the content of the passed variable later in their code as well and expect it to be unchanged. We could also make that a simple oneliner:

base.default_call('system.deleteSystems',[*system_ids].collect{|i| i.to_i })

  • Can you provide a spec for it? While the specs might be questionable as we generally mock the call, it will still provide an example of what we expect to get from the API and how the method should be used.

Thanks!

@bflad
Copy link
Contributor Author

bflad commented Dec 2, 2012

Added your change to use collect and not map! Also, added some spec tests for system delete which are green on my machine.

@duritong
Copy link
Owner

duritong commented Dec 2, 2012

Thanks! One last thing: [*system_ids] makes that the test on Array is not anymore needed:

ree-head :001 > a = 1
 => 1 
ree-head :002 > b = [ a ]
 => [1] 
ree-head :003 > [*a] == [*b]
 => true 
ree-head :004 > [*a]
 => [1] 
ree-head :005 > [*b]
 => [1] 

@bflad
Copy link
Contributor Author

bflad commented Dec 2, 2012

Nice, didn't know about that operator before. Thanks!

duritong added a commit that referenced this pull request Dec 2, 2012
Added delete function for system(s).
@duritong duritong merged commit 9e84c80 into duritong:master Dec 2, 2012
@duritong
Copy link
Owner

duritong commented Dec 2, 2012

Thanks!

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.

None yet

2 participants