Fix Command line arguments incorrectly echoed in TRACE

[JasonLG1979]

Fix up for #886
Closes: #898

[ashthespy]

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]

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]

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]

@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]

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]

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]

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

[JasonLG1979]

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]

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

[JasonLG1979]

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]

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

[JasonLG1979]

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]

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

[JasonLG1979]

Ok so I added 2 commits.

1. 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.

2. 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]

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]

@ashthespy and @kingosticks What say you?

[roderickvd]

I never know when you’re done 😆

[JasonLG1979]

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]

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]

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

[JasonLG1979]

After this I'll fix that alsa mixer bug.

[kingosticks]

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

[JasonLG1979]

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

It's all good.

[JasonLG1979]

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.

[JasonLG1979]

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]

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

[JasonLG1979]

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

[JasonLG1979]

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]

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

[roderickvd]

Alright let's get this in!