Improve support for keyword-positional arguments #254
Conversation
- Split up repeated code to improve readability - Add flag to identify_named_ports to allow for the addition of positional arguments to keyword argument dicionaries (required for the PyFunc drops to work).
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @myxie - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| logger.debug("Checking against keyargs: %s", keyargs) | ||
| portargs = {} | ||
| posargs = list(posargs) | ||
| logger.debug("Checking against keyargs: %s", keywordArgs) |
There was a problem hiding this comment.
suggestion: Consider using f-strings for logging
Using f-strings for logging can improve readability and performance. For example: logger.debug(f"Checking against keyargs: {keywordArgs}").
| logger.debug("Checking against keyargs: %s", keywordArgs) | |
| logger.debug(f"Checking against keyargs: {keywordArgs}") |
There was a problem hiding this comment.
This suggestion is incorrect and conflicts with pylint and the Python documentation.
| positionalArgs = _get_args(appArgs, positional=True) | ||
| keywordArgs = _get_args(appArgs, positional=False) |
There was a problem hiding this comment.
suggestion: Combine _get_args calls
Consider combining the two _get_args calls into a single line for better readability: positionalArgs, keywordArgs = _get_args(appArgs, positional=True), _get_args(appArgs, positional=False).
| positionalArgs = _get_args(appArgs, positional=True) | |
| keywordArgs = _get_args(appArgs, positional=False) | |
| positionalArgs, keywordArgs = _get_args(appArgs, positional=True), _get_args(appArgs, positional=False) |
There was a problem hiding this comment.
I do not agree that this adds readability.
|
@awicenec This is the work I've done to give us (optional) positional-and-keyword arguments. Would you mind providing a review? |
|
@awicenec just a 'bump' to confirm you're happy with these changes? |
|
Hi Ryan,
Yes, please go ahead and merge these.
Andreas
…On Thu, 6 June 2024, 03:00 Ryan Bunney, ***@***.***> wrote:
@awicenec <https://github.com/awicenec> just a 'bump' to confirm you're
happy with these changes?
—
Reply to this email directly, view it on GitHub
<#254 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJVMSARIWKRX3LDRBHLRXTZF67ERAVCNFSM6AAAAABH7DMJZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJRGI3TKMZXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Improve support for keyword-positional arguments
Issue
Previously, we addressed the issue that the positional parameter option currently does not work for Docker and Bash applications; a temporary fix was introduced in #248.
Unfortunately, that solution did not allow for the potential Keyword-and-Positional argument combination, which is used in the
PyFuncDROP classes. By commenting out the addition toportargsto fix positional arguments for Docker and Bash apps, I inadvertently broke the PyFunc apps.Solution
I have added a flag to the
identify_named_portsmethods that allows for the addition of positional arguments to the keyword argument dictionary, which required for the PyFunc drops to work.I have also added some support functions to reduce the incidence of duplicated coded in
replace_named_ports, to improve readability and make a bit more explicit that we are doing a lot of in-place manipulation of dictionaries (as opposed to modifying and returning them).