fix: properly support spaces in string values #27
Conversation
alexander-fenster
requested review from
vam-google and
viacheslav-rostovtsev
google-cla
bot
added
the
cla: yes
|
please just add comments everywhere explaining why this stuff is being done |
| @@ -92,7 +95,8 @@ public String getAttribute(Path bazelBuildFile, String target, String attribute) | |||
| // immediately. | |||
| public void setAttribute(Path bazelBuildFile, String target, String attribute, String value) | |||
| throws IOException { | |||
| execute(bazelBuildFile, String.format("set %s \"%s\"", attribute, value), target); | |||
| final String escapedValue = value.replace(" ", "\\ "); | |||
| execute(bazelBuildFile, String.format("set %s \"%s\"", attribute, escapedValue), target); | |||
There was a problem hiding this comment.
minor: just inlining replace call here would be just fine.
On a side note, it feels like setAttribute(), addAttribute(), batchSetAttribute(), batchAddAttribute() could become a one method, with an argument maybe.
There was a problem hiding this comment.
You know what, we don't need non-batch methods at all. We don't use them anywhere so YAGNI. They are terribly slow since they execute buildozer for each invocation.
| @@ -82,6 +82,9 @@ public String getAttribute(Path bazelBuildFile, String target, String attribute) | |||
| if (value.equals("(missing)")) { | |||
| return null; | |||
| } | |||
| if (value.charAt(0) == '"' && value.charAt(value.length() - 1) == '"') { | |||
| value = value.substring(1, value.length() - 1); | |||
There was a problem hiding this comment.
This does not seem to be related to the spaces issue. How did this thing work before and why is it needed?
Minor: another lazy option of doign it coudl be value.startsWith("\"") && value.endsWith("\""), though the currenv version is more "performant" as it works directly with characters (pretending like it matters here...).
There was a problem hiding this comment.
It is related. I will add a comment :) The thing is:
fenster@fenster ws:~/googleapis$ buildozer 'print ruby_cloud_title' //google/api/servicecontrol/v1:servicecontrol_ruby_gapic
"Service Control API V1"
fenster@fenster ws:~/googleapis$ buildozer 'print name' //google/api/servicecontrol/v1:servicecontrol_ruby_gapic
servicecontrol_ruby_gapic
So it adds quotes if the string has spaces inside. We don't need those quotes, so I'm stripping them.
There was a problem hiding this comment.
Added comment
Some values can have spaces in them. For such strings,
buildozer printreturns a"string in quotes", but to write them back, we"must\ quote\ spaces". Weird it is.