fix: if requested, properly encode Path types as base64 #84
Conversation
📝 WalkthroughWalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FormatCLIValue
participant MaybeEncode
participant Base64Encoder as encode_as_base64
participant ShlexQuote as shlex.quote
Caller->>FormatCLIValue: Call format_cli_value(value, base64_encode)
FormatCLIValue->>MaybeEncode: Invoke maybe_encode(value, base64_encode)
alt base64_encode == True
MaybeEncode->>Base64Encoder: Call encode_as_base64(str(value))
Base64Encoder-->>MaybeEncode: Return encoded string
else base64_encode == False
MaybeEncode->>ShlexQuote: Call shlex.quote(str(value))
ShlexQuote-->>MaybeEncode: Return quoted string
end
MaybeEncode-->>FormatCLIValue: Return processed string
FormatCLIValue-->>Caller: Return final value
Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snakemake_interface_executor_plugins/utils.py (2)
55-58: Consider updating the docstring to mention Path objects.The current docstring only mentions that str values are encoded when base64_encode is True, but the function now also handles Path objects in the same way. Consider updating the docstring to explicitly mention that both string and Path objects are encoded.
"""Format a given value for passing it to CLI. -If base64_encode is True, str values are encoded and flagged as being base64 encoded. +If base64_encode is True, str and Path values are encoded and flagged as being base64 encoded. """
66-69: Consider reusing the maybe_encode helper function for consistency.For consistency with how strings are handled in this function, consider reusing the
maybe_encodehelper for Path objects. This would make the implementation more consistent across different value types.if base64_encode: - return encode_as_base64(str(value)) + return maybe_encode(str(value)) else: return shlex.quote(str(value))Alternatively, for a cleaner approach:
-if base64_encode: - return encode_as_base64(str(value)) -else: - return shlex.quote(str(value)) +path_str = str(value) +return maybe_encode(path_str) if base64_encode else shlex.quote(path_str)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_interface_executor_plugins/utils.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
snakemake_interface_executor_plugins/utils.py
🔇 Additional comments (1)
snakemake_interface_executor_plugins/utils.py (1)
66-69: LGTM: Path objects now correctly support base64 encoding.The changes correctly extend the base64 encoding functionality to Path objects when the
base64_encodeflag is set toTrue. This implementation aligns with the PR title's objective of properly encoding Path types as base64 when requested.
Summary by CodeRabbit