-
-
Notifications
You must be signed in to change notification settings - Fork 490
#1544 Changed the daemon output from json to text
#1570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Closes #1544 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Should the code in the test file needs to be modified? It still allows json as logFormat.
arduino-ide/arduino-ide-extension/src/test/node/arduino-daemon-impl.test.ts
Lines 27 to 41 in a36524e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
The output is also more readable IMO
from:
daemon INFO {"level":"info","msg":"Required tool","time":"2022-10-21T10:31:19+02:00","tool":{"ToolName":"arduinoOTA","ToolVersion":"1.2.1","ToolPackager":"arduino"}}
{"level":"info","msg":"Required tool","time":"2022-10-21T10:31:19+02:00","tool":{"ToolName":"arm-none-eabi-gcc","ToolVersion":"7-2017q4","ToolPackager":"arduino"}}
{"level":"info","msg":"Required tool","time":"2022-10-21T10:31:19+02:00","tool":{"ToolName":"bossac","ToolVersion":"1.7.0-arduino3","ToolPackager":"arduino"}}
{"level":"info","msg":"Required tool","time":"2022-10-21T10:31:19+02:00","tool":{"ToolName":"openocd","ToolVersion":"0.10.0-arduino7","ToolPackager":"arduino"}}
to:
daemon INFO INFO[0009] Searching tools required for board arduino:samd:mkrwifi1010
INFO[0009] Required tool tool="arduino:CMSIS@4.5.0"
INFO[0009] Required tool tool="arduino:CMSIS-Atmel@1.2.0"
INFO[0009] Required tool tool="arduino:arduinoOTA@1.2.1"
INFO[0009] Required tool tool="arduino:arm-none-eabi-gcc@7-2017q4"
INFO[0009] Required tool tool="arduino:bossac@1.7.0-arduino3"
INFO[0009] Required tool tool="arduino:openocd@0.10.0-arduino7"
About the comment from nmzaheer: yes we should probably remove that flag from the test file too – nice catch, thank you for taking the time to look into the code!
Tests aren't failing anyway, so I won't request changes (in case @kittaakos you left that flag there on purpose for some reason)
Do you want me to remove test?
Do you want me to remove test?
Not really. For me it's the same, it doesn't really change anything I guess.
OK. I will create a follow-up to clean up the text|json tests. There are multiple remarks:
- initially, the CLI did not print out the daemon port, so it had to work with both
jsonandtext. That's why we had tests. - currently, it does not matter whether
textorjsonis given after the--log-formatbecause the address format is dictated by thedaemon --format jsonmini|text|jsonflags.
Motivation
To avoid encoding text to JSON on the CLI side.
Change description
Dropped the
--log-format jsonoptions from the daemon spawn args.Other information
Closes #1544
Reviewer checklist