search

ansible-collections / community.rabbitmq

Manage RabbitMQ with Ansible
http://galaxy.ansible.com/community/rabbitmq
Other
31 stars 50 forks source link

Refactor `_exec` parameter `run_in_check_mode` for a name that improves readability #89

Closed aitorpazos closed 3 years ago

aitorpazos commented 3 years ago

This PR has been triggered by this comment in PR #65 (thanks to @Andersson007).

Hopefully it improves the code readability by renaming run_in_check_mode parameter to force_exec_in_check_mode in the _exec function used in multiple modules.

Andersson007 commented 3 years ago

@aitorpazos thanks for the PR! (btw fyi I'm going to release 1.1.0 within several days which will contain your module, thanks)

And also as we can see the modules share the same function. We could move it (and other shared things if exist), to plugins/module_utils/something.py, document it there, and use everything instead of having the code duplicated.

  1. It could be a parental class but i personally prefer composition.
  2. Or it could be just a function, say, rabbitmqctl_exec (obj, args, force_exec_in_check_mode=False) where obj is an argument via which the objects will pass themselves.

the class could look like:

class RabbitMQ():
    """Doc explaining purpose, args, etc."""
    def __init__(self, obj):
    self. obj = obj

    def exec(self, args, force_exec_in_check_mode=False):
        """Doc explaining purpose, args, etc."""
        if not self.obj.module.check_mode or (self.obj.module.check_mode and force_exec_in_check_mode):
            cmd = [self.obj._rabbitmqctl, '-q', '-n', self.obj.node]
            rc, out, err = self.obj.module.run_command(cmd + args, check_rc=True)
            return out.splitlines()
        return list()

in the target classes you're changing here in __init__ we could initialize `self.rabbitmq = RabbitMQ(self)

and then _exec() method could look like:

    def _exec(self, args, force_exec_in_check_mode=False):
        return self.rabbitmq.exec(args, force_exec_in_check_mode)

Just an idea.

@aitorpazos @odyssey4me what do you think?

Andersson007 commented 3 years ago

I'm also OK with merge this as is and then, provided that the idea is interesting to you, @aitorpazos could implement it in a separate PR (or if you don't have time / wish, I could)

aitorpazos commented 3 years ago

I agree, that would be cleaner, but I'll need to get some more time... (probably this weekend) We can go with option 2 PS: @Andersson007 sorry for not being able to help reviewing PR 83 yesterday

Andersson007 commented 3 years ago

I agree, that would be cleaner, but I'll need to get some more time... (probably this weekend) We can go with option 2 PS: @Andersson007 sorry for not being able to help reviewing PR 83 yesterday

Cool, sounds good to me. And no problem with PR 83! Let's wait for Jesse's feedback for a couple of days.

Andersson007 commented 3 years ago

and take your time, no rush with this at all

Andersson007 commented 3 years ago

@aitorpazos thanks for the contribution! @odyssey4me thanks for reviewing!