Fix Command line arguments incorrectly echoed in TRACE #899

JasonLG1979 · 2021-12-05T19:03:46Z

Fix up for #886
Closes: #898

ashthespy · 2021-12-06T10:35:51Z

Any particular reason why you are parsing args again, and not using the parsed matches struct?

My 0.2c, if you are going through all this trouble to define these constants, why not make it more structured and use clap?

JasonLG1979 · 2021-12-06T13:42:14Z

Any particular reason why you are parsing args again, and not using the parsed matches struct?

My 0.2c, if you are going through all this trouble to define these constants, why not make it more structured and use clap?

Because you can't tell the difference between options and flags in getopts. You can ofc call matches.opt_present on either but that doesn't tell you option or flag and you can call matches.opt_str on a option but there is no safe way to ask getopts "hey is this an option or a flag and if it's an option I want it's value." Calling matches.opt_str on a flag just panics it doesn't throw an error. I will look into clap.

JasonLG1979 · 2021-12-06T14:17:05Z

Another option I was looking into is making arrays of struct's and using them to keep the options and flags organized, make OPT_FLAGS and OPT_OPTION const's and just looping though them to create the optflag and optopt.

struct OptFlag<'a> {
    short_name: &'a str,
    long_name: &'a str,
    desc: &'a str,
}

struct OptOption<'a> {
    short_name: &'a str,
    long_name: &'a str,
    desc: &'a str,
    hint: &'a str,
}

...

    const OPT_FLAGS: &[OptFlag] = &[
        ...
    ];
    
    const OPT_OPTIONS: &[OptOption] = &[
        ...
    ];
    
    for flag in OPT_FLAGS {
        opts.optflag(
            flag.short_name,
            flag.long_name,
            flag.desc,
        );
    }
    
    for option in OPT_OPTIONS {
        opts.optopt(
            option.short_name,
            option.long_name,
            option.desc,
            option.hint,
        );
    }

After that it's trivial to tell if something is an option or a flag and to get all the fields for the thing as neither getopts nor clap is capable of doing that as far as I can tell. From what I can tell once you dump the info into either you're pretty much on your own.

I believe the above approach kinda goes toward the "smart data structures, dumb code" direction.

JasonLG1979 · 2021-12-06T16:40:21Z

@ashthespy I looked at clap and other than the .yml functionality it doesn't look much different than getopts really. It doesn't solve my particular problem of a safe and easy way to do:

        let get_value = |arg: &str| -> String {
            let arg = strip_arg(arg);

            if matches!(
                arg.as_str(),
                PASSWORD | PASSWORD_SHORT | USERNAME | USERNAME_SHORT
            ) {
                "XXXXXXXX".to_string()
            } else if VALID_OPTIONS.contains(&&*arg) {
                matches.opt_str(&arg).unwrap_or_else(|| "".to_string())
            } else {
                "".to_string()
            }
        };

The if VALID_OPTIONS.contains(&&*arg) bit is the REALLY important part.

roderickvd · 2021-12-06T21:08:50Z

I agree that clap is the way forward for the librespotd "the player". Maybe clap doesn't have functionality to dump everything to stdout (or maybe it does -- I haven't thought it out or checked if it simply implements Debug), but it does support the environment parsing functionality that you built yourself. It's widely used and robust. Looking at #899 (comment), it doesn't make sense to build increasingly complex stuff on top off getopts when there's a great crate for it.

JasonLG1979 · 2021-12-06T21:36:46Z

I agree that clap is the way forward for the librespotd "the player". Maybe clap doesn't have functionality to dump everything to stdout (or maybe it does -- I haven't thought it out or checked if it simply implements Debug), but it does support the environment parsing functionality that you built yourself. It's widely used and robust. Looking at #899 (comment), it doesn't make sense to build increasingly complex stuff on top off getopts when there's a great crate for it.

So what do you want me to do then? Do you want me to rewrite with clap? Do you want to leave it as-is or do you want to try to fix it? You gotta give me a direction.

roderickvd · 2021-12-06T21:41:01Z

If you would spare the time and effort, I'd be great if you would move it to clap.

JasonLG1979 · 2021-12-06T21:54:55Z

If you would spare the time and effort, I'd be great if you would move it to clap.

I will put it on my list,lol!!!

JasonLG1979 · 2021-12-06T21:56:23Z

I will set this a draft and when I have something worth reviewing I will let you know.

JasonLG1979 · 2021-12-07T03:26:31Z

There you go @roderickvd and @ashthespy a fix up that doesn't require defining new const's. I found the functions in getopts that I needed.

With this PR the args are echo'd better.

--verbose "totally invalid because --verbose is a flag" --name --name -N "totally invalid because -N is a flag" --normalisation-pregain -3.0 -d -a -u stuff fart

Out puts what you'd expect and isn't tripped up by any weird corner cases that I can tell:

[2021-12-07T03:14:17Z TRACE librespot] Command line argument(s):
[2021-12-07T03:14:17Z TRACE librespot] 		--verbose 
[2021-12-07T03:14:17Z TRACE librespot] 		--name --name
[2021-12-07T03:14:17Z TRACE librespot] 		-N 
[2021-12-07T03:14:17Z TRACE librespot] 		--normalisation-pregain -3.0
[2021-12-07T03:14:17Z TRACE librespot] 		-d -a
[2021-12-07T03:14:17Z TRACE librespot] 		-u XXXXXXXX

I'd like to see this merged in the meantime until I implement clap as that's basically going to be another rewrite of main and I just finished one not that long ago,lol!!!

roderickvd · 2021-12-07T06:06:23Z

Yeah, this is much sweeter in the meantime. Ready to go for you? If so I’ll check it out tonight.

JasonLG1979 · 2021-12-07T06:37:09Z

Yeah, this is much sweeter in the meantime. Ready to go for you? If so I’ll check it out tonight.

There I think so. I was digging though all the str and String methods trying to pair it down as much as possible. I think that will do it.

JasonLG1979 · 2021-12-07T06:40:51Z

It actually ended up being less lines of code than the broken version,lol!!!

src/main.rs

JasonLG1979 · 2021-12-08T03:41:51Z

Ok so I added 2 commits.

Make it so librespot doesn't silently ignore non-Unicode while parsing env vars. It does silently ignore invalid keys. We don't want some random env var that's not even meant for us to crash librespot. That would be bad.
Don't panic when gathering up the command line args. Iterating over std::env::args will panic! if you hit a invalid Unicode. Let's not do that.

JasonLG1979 · 2021-12-08T17:38:56Z

The last commit guards against empty strings for args that accept strings like name. Before it was possible to do, for example -n "" and librespot would just hang and do nothing but output libmdns warnings because an empty string is not a valid name.

JasonLG1979 · 2021-12-08T19:23:14Z

@ashthespy and @kingosticks What say you?

roderickvd · 2021-12-09T06:14:21Z

I never know when you’re done 😆

JasonLG1979 · 2021-12-09T06:19:30Z

I never know when you’re done

I get antsy and see more and more as I look at the code. I'll be done if you're willing to review what I got?

JasonLG1979 · 2021-12-09T06:20:21Z

I was just squashing it on my side. Let know if you'd like me to push the squash before you review it or not?

JasonLG1979 · 2021-12-09T06:22:02Z

99% of the commit's are clean ups and minor refactoring. There's really not much there.

JasonLG1979 · 2021-12-09T06:26:46Z

After this I'll fix that alsa mixer bug.

src/main.rs

kingosticks · 2021-12-09T15:22:09Z

Apologies but I don't have time for a real code review, just a couple of drive-by comments.

JasonLG1979 · 2021-12-09T15:27:17Z

Apologies but I don't have time for a real code review, just a couple of drive-by comments.

It's all good.

JasonLG1979 · 2021-12-10T21:04:12Z

Ok @roderickvd I'm done I swear, cross my little black heart,lol!!! I've deleted the local clone on my machine. Feel free to review at your leisure. I know you're busy with the new API and I know this looks like a lot at 1st glance but it's really mostly just a bunch of really small clean ups. I don't even think it warrants an entry in the change log.

roderickvd

Phew this has become pretty complex for what seems so easy!
Sorry to keep bugging you about this, I know it's taking time.

src/main.rs

Fix up for #886 Closes: #898 And... * Don't silently ignore non-Unicode while parsing env vars. * Iterating over `std::env::args` will panic! on invalid unicode. Let's not do that. `getopts` will catch missing args and exit if those args are required after our error message about the arg not being valid unicode. * Gaurd against empty strings. There are a few places while parsing options strings that we don't immediately evaluate their validity let's at least makes sure that they are not empty if present. * `args` is only used in `get_setup` it doesn't need to be in main. * Nicer help header. * Get rid of `use std::io::{stderr, Write};` and just use `rpassword::prompt_password_stderr`. * Get rid of `get_credentials` it was clunky, ugly and only used once. There is no need for it to be a separate function. * Handle an empty password prompt and password prompt parsing errors. * + Other random misc clean ups.

JasonLG1979 · 2021-12-12T23:35:57Z

There you go @kingosticks that last commit DRYs up a lot of the log messaging.

The magic happens with:

    let invalid_error_msg =
        |long: &str, short: &str, invalid: &str, valid_values: &str, default_value: &str| {
            error!("Invalid `--{}` / `-{}`: \"{}\"", long, short, invalid);

            if !valid_values.is_empty() {
                println!("Valid `--{}` / `-{}` values: {}", long, short, valid_values);
            }

            if !default_value.is_empty() {
                println!("Default: {}", default_value);
            }
        };

JasonLG1979 · 2021-12-13T04:50:01Z

@roderickvd I'd also like a review of the last commit on this.

JasonLG1979 · 2021-12-13T22:57:53Z

The last commit condenses option parsing for number type options that have a set range.

src/main.rs

JasonLG1979 · 2021-12-15T18:39:20Z

After this I plan on releasing a new Raspotify version. That should be a good scream test for the last few commits since the last release.

JasonLG1979 · 2021-12-15T18:40:52Z

Especially since the next version of Raspotify leans pretty heavy on the env parsing feature.

roderickvd · 2021-12-16T21:49:03Z

Alright let's get this in!

roderickvd added the bug label Dec 6, 2021

roderickvd assigned roderickvd and JasonLG1979 Dec 6, 2021

JasonLG1979 marked this pull request as draft December 6, 2021 21:56

JasonLG1979 marked this pull request as ready for review December 7, 2021 03:26

roderickvd reviewed Dec 7, 2021

View reviewed changes

src/main.rs Show resolved Hide resolved

kingosticks reviewed Dec 9, 2021

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

roderickvd reviewed Dec 11, 2021

View reviewed changes

src/main.rs Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

Dry up error messages.

d29337c

condense some option parsings

368bee1

JasonLG1979 requested review from roderickvd and kingosticks December 14, 2021 20:10

roderickvd reviewed Dec 15, 2021

View reviewed changes

src/main.rs Show resolved Hide resolved

Merge branch 'librespot-org:dev' into arg_parse_fixup

306096e

roderickvd merged commit e5fd7d6 into librespot-org:dev Dec 16, 2021

JasonLG1979 deleted the arg_parse_fixup branch December 16, 2021 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Command line arguments incorrectly echoed in TRACE #899

Fix Command line arguments incorrectly echoed in TRACE #899

JasonLG1979 commented Dec 5, 2021

ashthespy commented Dec 6, 2021 •

edited

Loading

JasonLG1979 commented Dec 6, 2021 •

edited

Loading

JasonLG1979 commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021 •

edited

Loading

roderickvd commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021 •

edited

Loading

roderickvd commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021

JasonLG1979 commented Dec 7, 2021 •

edited

Loading

roderickvd commented Dec 7, 2021

JasonLG1979 commented Dec 7, 2021

JasonLG1979 commented Dec 7, 2021

JasonLG1979 commented Dec 8, 2021

JasonLG1979 commented Dec 8, 2021 •

edited

Loading

JasonLG1979 commented Dec 8, 2021

roderickvd commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

kingosticks commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 10, 2021 •

edited

Loading

roderickvd left a comment

JasonLG1979 commented Dec 12, 2021 •

edited

Loading

JasonLG1979 commented Dec 13, 2021

JasonLG1979 commented Dec 13, 2021

JasonLG1979 commented Dec 15, 2021

JasonLG1979 commented Dec 15, 2021

roderickvd commented Dec 16, 2021

Fix Command line arguments incorrectly echoed in TRACE #899

Fix Command line arguments incorrectly echoed in TRACE #899

Conversation

JasonLG1979 commented Dec 5, 2021

ashthespy commented Dec 6, 2021 • edited Loading

JasonLG1979 commented Dec 6, 2021 • edited Loading

JasonLG1979 commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021 • edited Loading

roderickvd commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021 • edited Loading

roderickvd commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021

JasonLG1979 commented Dec 6, 2021

JasonLG1979 commented Dec 7, 2021 • edited Loading

roderickvd commented Dec 7, 2021

JasonLG1979 commented Dec 7, 2021

JasonLG1979 commented Dec 7, 2021

JasonLG1979 commented Dec 8, 2021

JasonLG1979 commented Dec 8, 2021 • edited Loading

JasonLG1979 commented Dec 8, 2021

roderickvd commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

kingosticks commented Dec 9, 2021

JasonLG1979 commented Dec 9, 2021

JasonLG1979 commented Dec 10, 2021 • edited Loading

roderickvd left a comment

Choose a reason for hiding this comment

JasonLG1979 commented Dec 12, 2021 • edited Loading

JasonLG1979 commented Dec 13, 2021

JasonLG1979 commented Dec 13, 2021

JasonLG1979 commented Dec 15, 2021

JasonLG1979 commented Dec 15, 2021

roderickvd commented Dec 16, 2021

ashthespy commented Dec 6, 2021 •

edited

Loading

JasonLG1979 commented Dec 6, 2021 •

edited

Loading

JasonLG1979 commented Dec 6, 2021 •

edited

Loading

JasonLG1979 commented Dec 6, 2021 •

edited

Loading

JasonLG1979 commented Dec 7, 2021 •

edited

Loading

JasonLG1979 commented Dec 8, 2021 •

edited

Loading

JasonLG1979 commented Dec 10, 2021 •

edited

Loading

JasonLG1979 commented Dec 12, 2021 •

edited

Loading