Fix #1215 Output sketch memory usage in non-verbose mode #1383
Conversation
AlexIII
force-pushed
the
fix_no_verbose_sketch_stats_output
branch
2 times, most recently
from
db5f211 to
1955f38
Compare
src/arduino/arduino.ts
Outdated
| @@ -732,6 +732,11 @@ export class ArduinoApp { | |||
| } | |||
| if (verbose) { | |||
| arduinoChannel.channel.append(line); | |||
| } else { | |||
| // Output sketch memory usage in non-verbose mode | |||
| if (line.startsWith("Sketch uses ") || line.startsWith("Global variables use ")) { | |||
There was a problem hiding this comment.
I suggest replacing these two tests with a isMemoryUsageInformation(line) function call instead.
First, this ought to be localizable since the output from arduino might not be in English, e.g. from https://github.com/arduino/Arduino/blob/master/arduino-core/src/processing/app/i18n/Resources_tr.po:
msgid ""
"Sketch uses {0} bytes ({2}%%) of program storage space. Maximum is {1} "
"bytes."
msgstr "Çalışmanız programın {0} bayt ({2} %%) saklama alanını kullandı. Maksimum {1} bayt."
Now, only supporting English as a first step in the beginning might be ok, but at least i18n should be on the map and not forgotten.
And secondly it is just good separation of concerns to split out the parsing details to a separate function. What if the output text changes slightly in newer versions of arduino? There is no logic here in this function that should require any change because of that.
There was a problem hiding this comment.
I agree, the solution is not ideal, this was meant as a hotfix (that may be improved later) in hopes it could resolve the issue for users now.
I will separate the condition into a separate function, as you've suggested.
There was a problem hiding this comment.
The isMemoryUsageInformation function looks good. But I have no authority to approve this pull request if that's your question. I have just contributed a few things before.
AlexIII
force-pushed
the
fix_no_verbose_sketch_stats_output
branch
from
1955f38 to
0295850
Compare
There was a problem hiding this comment.
Thanks for your contribution! I'm comfortable merging this as English-only for now to unblock you and because it looks like there are other hard-coded English filters in the codebase (like /^Picked\sup\sJAVA_TOOL_OPTIONS:\s+/ just below your change), so it makes sense to address that all at once when we have a better plan for internationalizing this extension. I opened #1417 to track the remaining work.
Otherwise, these changes look good to me. I'll merge it as soon as the CI checks finish.
I realize this implementation may not be the best, but it should work fine.
You can implement a better solution when you'll have time, meanwhile many people need this fixed now.