OceanDataTools / openrvdas

An open source data acquisition system designed for use on research vessels and other scientific installations
http://openrvdas.org
Other
39 stars 20 forks source link

CLI parameter order matters a bit too much in `listen.py` #353

Closed veggiemike closed 9 months ago

veggiemike commented 10 months ago

There are a couple important "global" parameters for listen.py that change the behavior of the Readers/Writers we create. Namely, --verbose, --encoding, and --network_eol. If you specify any of them AFTER specifying any readers or writers, the readers/writers get instantiated before the global parameters get parsed, resulting in very confused users.

Most other applications that do as much from the command line as listen.py does (at least, that I've used), allow for these types of parameters to be provided anywhere on the command line. You end up with parsing command line args as one complete step, then follow through in a 2nd block of code instantiating things once ALL the parameters have been parsed... So it would be a bit of a messy change for listen.py. Certainly doable, though.

It's bitten me once, so I'm sure it's going to happen to other users at some point.

veggiemike commented 10 months ago

Looking at this again w/ fresh eyeballs, this was mostly just "user error". I've read through all the argument parsing code, and the disclaimer note on the bottom of --help output, and retested. Looks like --verbose works fine wherever you put it (not sure why I thought it didn't), but --encoding and --network_eol have to come before the readers/writers they're for, which is exactly what the usage message says at the bottom. Ha.

That being said, I'd still argue that --encoding in particular needs to be specified exactly once and can't change between parsing segments. If you change encoding between them, you'll just make Python spit out TypeError: a bytes-like object is required, not 'str' on the very first input read().

davidpablocohn commented 10 months ago

Ah yes, the argument order in the listen.py will plague me to my death. The original reason for implementing it with order dependence was so that things like "--parse_definition_path" got applied to the right ParseTransform in case there was more than one (yes, unlikely, I know). Were I to reimplement from scratch, I think I would make any module that required more than one parameter to take them the way --serial does: as comma-separated '=' pairs.

On Mon, Oct 30, 2023 at 7:04 PM Michael D Labriola @.***> wrote:

Looking at this again w/ fresh eyeballs, this was mostly just "user error". I've read through all the argument parsing code, and the disclaimer note on the bottom of --help output, and retested. Looks like --verbose works fine wherever you put it (not sure why I thought it didn't), but --encoding and --network_eol have to come before the readers/writers they're for, which is exactly what the usage message says at the bottom. Ha.

That being said, I'd still argue that --encoding in particular needs to be specified exactly once and can't change between parsing segments. If you change encoding between them, you'll just make Python spit out TypeError: a bytes-like object is required, not 'str' on the very first input read().

— Reply to this email directly, view it on GitHub https://github.com/OceanDataTools/openrvdas/issues/353#issuecomment-1786320577, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFO7V3R2ZCSZTFE4355GDQ3YCBMDJAVCNFSM6AAAAAA6WP5FBGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGMZDANJXG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>