fix handling of \n in submission cli --add-file= option
#6504
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: A conditional in Jobspec.add_file() checks for a newline in the data argument before checking that encoding is None, but the check should only be done when encoding is None. Reverse the order of the conditional so that newline is checked only when encoding is None.
Problem: The code to handle --add-file arguments is all within a condditional, but this code would be better moved to a function for readability. Add a function for handling `--add-file` args.
garlick
approved these changes
Dec 13, 2024
| @@ -969,7 +969,10 @@ def init_jobspec(self, args): | |||
|
|
|||
There was a problem hiding this comment.
commit message: s/indicaates/indicates/
Problem: Documentation for the `--add-file=NAME=DATA` option indicates that DATA will be treated as a path unless a newline appears in DATA. However, the obvious doesn't work: --add-file=test='this is a test file\n' because what Python actually gets is an escaped newline '\\n', so test for '\n' in the string fail. Convert any escaped newlines to actual newlines in handle_add_file_arg() so that subsequent detection of newline works.
Problem: No test in the testsuite ensures that the submission cli `--add-file` option properly detects '\n' in DATA and treats it like file data instead of a path. Add a test to t2710-python-cli-submit.t.
grondo
force-pushed
the
add-file-data
branch
from
5f28bb3 to
ad07a9e
Compare
|
Thanks! Fixed that and will set MWP. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a small bug in the submission cli
--add-file=[NAME=]DATAoption. The option is documented to allow file data on the command line ifDATAcontains a\n. However, any\npassed on the command line ends up quoted, so this doesn't actually work:This PR just unescapes in newlines in
DATAso that newline detection works (and the newlines appear as newlines in the resulting fileref data).