Replace '-' in bash variables with '_' #129
Conversation
… they have "-" in their name.
…iable names in bash, but not other api end points.
| @@ -160,7 +160,7 @@ trait ApiResponse extends Controller { | |||
| v match { | |||
| case m: JsObject => formatBashResponse(m, "%s_".format(prefix + k)) | |||
| case JsArray(list) => formatList(list, "%s_".format(prefix + k)) | |||
| case o => "%s%s=%s;".format(prefix, k, formatBasic(o)) | |||
| case o => "%s%s=%s;".format(prefix, k.replace('-', '_'), formatBasic(o)) //Variables with '-' in their name are invalid for bash. | |||
There was a problem hiding this comment.
This seems kinda hacky. There are other chars that make the key not a valid POSIX environment variable name; we should handle them all. Ill make a branch with some changes.
|
#130 is the right way to do this. |
|
Didn't realizing #130 was pushing into this PR's branch. |
…-variables add formatPosixKey function
|
May wanna add come other test cases, i.e. attributes starting with a number -> |
|
Right now, the test that we do have is a bit hacky. IMHO, we shouldn't really be testing this functionality here. But, I didn't think there were enough tests to justify its own test suite. If we really want to test full POSIX key correctness when we give back shellscript output, then yeah, we should probably make a separate test spec for it. |
|
|
||
| // formats a key to an acceptable POSIX environment variable name | ||
| def formatPosixKey(key: String): String = if (!key.isEmpty) { | ||
| ("""[^a-zA-Z_]""".r.replaceAllIn(key.head.toString,"_") + """[^a-zA-Z0-9_]""".r.replaceAllIn(key.tail,"_")).toUpperCase |
There was a problem hiding this comment.
@byxorna @Primer42 i'm not sure this is what we want here, i personally would rather get an exception if i ask for a bash response of an asset with an attribute of 99_balloons then get ___balloons. replacing special characters like - or | with underscores seems completely fine though.
There was a problem hiding this comment.
This would make 99_balloons -> _9_balloons, but its still not ideal. Silently nuking user data in the response is bad, so maybe we can preserve it. Lets just prefix the key with a _ if the first character isnt POSIX compliant. That way the full key is preserved, but made posix compliant by stamping on as few characters as possible. (Special chars like ! and - will still be made to _). Ill open a PR onto this branch with my fix
There was a problem hiding this comment.
@byxorna i agree, i think that's a really good solution. someone should also mention this in the docs as well.
There was a problem hiding this comment.
I'll add something about this to the API doc. PR imminent.
…-variables make formatPosixKey behave better
| val posixTailRegex = """[^a-zA-Z0-9_]""".r | ||
| key.head.toString match { | ||
| case posixHeadRegex() => formatPosixKey("_" + key) | ||
| case _ => posixTailRegex.replaceAllIn(key,"_").toUpperCase |
There was a problem hiding this comment.
@Primer42 would you mind fixing the spacing here?
|
lgtm 🍕 |
|
This PR currently breaks the text/x-shellscript endpoint. |
…inside of the call to formatPosixKey.
|
Found the bug and tested. LGTM 🎸 |
|
💴 +1 |
…variables Ensure variable names are valid POSIX names when using text/x-shellscript API endpoint.
…cript-variables Ensure variable names are valid POSIX names when using text/x-shellscript API endpoint.
…cript-variables Ensure variable names are valid POSIX names when using text/x-shellscript API endpoint.
Fixed bug reported in issue #118, where bash variables are invalid if they have "-" in their name.
@dallasmarlow @joshrabinowitz @byxorna