Ensure that Fact#command does not take global arguments #1115
Conversation
martenlienen
force-pushed
the
fact-name-arg
branch
from
8cb913d to
f12fbe6
Compare
heh, guess we know why the old handling existed! Lack of comments on that meant I had long forgotten the context which makes sense! This approach is cleaner, and although the global name argument clash is annoying it makes sense to keep things consistent between facts and operations. Not quite got time to review code right now but thank you for pulling this one together @martenlienen 🙏 |
There was a problem hiding this comment.
Thank you @martenlienen, this is great work. Also good to change this now ahead of 3.0 with the breaking arg change 🙏
| # Check that fact's `command` method does not inadvertently take a global | ||
| # argument, most commonly `name`. | ||
| if hasattr(cls, "command") and callable(cls.command): | ||
| command_args = set(inspect.signature(cls.command).parameters.keys()) |
There was a problem hiding this comment.
This is neat, will copy this to operations as well!
A recent commit c89e211 changed getting facts so that arguments to
FactBase#commandclashing with global arguments are filtered from the kwargs. This includes the often used argumentnameand facts likeDebPackagebroke as a result.This PR introduces a check that subclasses of
FactBasedon't take any global arguments in theircommandmethod and updates all existing facts and tests accordingly. There is also a tiny bit of improved shell quoting in the deb, pacman and rpm facts.