CLI Interface is not pipe-aware, and doesn't properly disable VT-100 codes when piped into another application

[fake-name]

I'd not normally call this sort of thing out, but the readme states:

Designed to work nicely with other standard UNIX tools (watch, socat, etc.), see the examples.

Well, it doesn't:

durr@shedmon ~> watch modbus /dev/ttyS5 -b 9600 -p 1 -s 2 -v i@0/10h
Every 2.0s: modbus /dev/ttyS5 -b 9600 -p 1 -s 2 -v i@0/10h                                                                                                                                             shedmon: Sun Jan 17 22:38:48 2021

^[90mParsed 0 registers definitions from 1 files^[39m
^[90m→ < 02 04 00 00 00 0a 70 3e >^[39m
^[90m← < 02 04 14 04 95 00 00 00 00 00 00 00 00 d1 95 00 0e 02 58 00 00 00 00 18 e4 > 25 bytes^[39m
^[90m← [1173, 0, 0, 0, 0, 53653, 14, 600, 0, 0]^[39m
0: (1173, 0, 0, 0, 0, -11883, 14, 600, 0, 0)

This looks to be because you're directly embedding color codes, rather then only including them conditionally if isatty() is true.

Convenience libraries (I like Colorama) automatically detect this, and all their color-code constants default to empty strings if no TTY is connected. Either doing that manually, or using a library like colorama would probably be a decent idea.

[favalex]

:+1: thanks for you report and your suggestion. If you plan to use this tool and this is important to you, do you by any chance have time to submit a PR? Otherwise I will do it when I have time.

[fake-name]

I guess the question is would you be OK with taking on an additional dependency (Colorama)?

[favalex]

I'm OK with an additional dependency, it's not like the tool is self-contained anyway.

Il giorno mar 19 gen 2021 alle ore 11:25 C W notifications@github.com ha scritto:

I guess the question is would you be OK with taking on an additional dependency (Colorama)?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/favalex/modbus-cli/issues/8#issuecomment-762749225, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAS4IFYMN4URQJL7EOWJBLS2VMXZANCNFSM4WG2ER5Q .

[favalex]

What do you think of sending the logging to stderr instead of stdout? That's what I've been doing recently. So far I haven't found any drawback.

[fake-name]

I'm OK with an additional dependency, it's not like the tool is self-contained anyway.

I've contributed to (or attempted to contribute to) projects that basically refused any PRs which added new dependencies. ¯\_(ツ)_/¯

What do you think of sending the logging to stderr instead of stdout? That's what I've been doing recently. So far I haven't found any drawback.

That's.... actively worse? That would completely break using watch, for example.

You should only ever write to stderr if you're outputting data that is machine-formatted to stdout, stdout is redirected, and you have out-of-bound errors you want to report.

[favalex]

I'm glad we discussed this, I think we roughly agree on the general idea of what stderr is for, but not on what belongs to stdout and stderr in this specific case.

As I see it, the only output of the tool is the content of the registers, and that should go to stdout (where it can potentially be picked up by other tools consuming it as input.) The logging is optional and ancillary information for the benefit of the human running the tool and could go to stderr without interfering with the stdout pipe.

It's easy enough to 2>&1 and bring the streams back together, if that's really intended (for example for watch.)

Does this approach make sense to you?

[fake-name]

Does this approach make sense to you?

stderr is for error output 99% of the time. It's generally only not for errors when you have a command that's consuming from, or piped into a file.

I don't think any output that's triggered by a -v flag should go to stderr. If someone want's to pipe the output into a command that's consuming it and it confuses that tool, they can just remove the -v.

Really, I don't find the output useful (for watch or similar) without a -v flag, because the hex view lets you detect things like changes (via watch --differences) because the hex view of the data is constant length. If you have a message with multiple sections that change continuously, effectively the entire line is just marked as changing continuously.

Another change that I'd think would be handy would be to feed the output through something like pprint.pprint(), to make it visually nicer for human consumption.

[favalex]

:+1: I'm OK with all the changes you proposed, and leaving stderr alone. If you have time feel free to implement them.

[fake-name]

I should be able to get to it this weekend.