fix(builder)!: Accept a more liberal Iterator

[epage]

This replaces the most common uses of &[] with IntoIterator.

This is based on #2414. Unlike that, this stays consistent with using
Into<&'help str>.

There are some remaining functions not converted over, like defaults and
requires.

BREAKING CHANGE: Instances like aliases(&[]) need to switch to
aliases([]).

[epage]

This worth it for 3.0? What more functions would we need to convert over for this to be considered "done"

[pksunkara]

Just one comment.

[pksunkara]

Suggested change

	pub fn value_names<I>(mut self, names: I) -> Self
	pub fn value_names<I, T>(mut self, names: I) -> Self

[epage]

I was generally aiming to be consistent with what the non-iterator versions accepted. In this case, value_name accepts a &str.

We can change that if we want.

[pksunkara]

BREAKING CHANGE: Instances like aliases(&[]) need to switch to
aliases([]).

I thought this wasn't needed. I guess that is why #2414 introduced AsRef<str> IIUC

[epage]

BREAKING CHANGE: Instances like aliases(&[]) need to switch to
aliases([]).

I thought this wasn't needed. I guess that is why #2414 introduced AsRef<str> IIUC

More so I think its because it did &T rather than T.

    where
        I: IntoIterator<Item = &'help T>,
        T: AsRef<str> + 'help,

[epage]

Looks like that wasn't needed and this can work

• Into version: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=96d727adfcb992f27af84fae2d279daf
• AsRef version: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bdabf777de171a4e8e7981e6ea37f190

Going to go do a PR to switch Into<&str> to AsRef in prep for this one

[pksunkara]

Do we need lifetime in that AsRef version? I don't see it in the second snippet.

[epage]

Bad news is we can't control the lifetime for AsRef so it won't work. In 4.0 when we re-evaluate our string type, this might be easier

[pksunkara]

Wouldn't the following work?

    where
        I: IntoIterator<Item = T>,
        T: AsRef<str> + 'help,

[epage]

That can work but what I was looking at was how to be consistent between singular and iterator functions. If we don't care about consistency, that is fine.

[pksunkara]

I was looking at was how to be consistent between singular and iterator functions.

I am not sure what you mean by that

[epage]

Currently, most cases that take a single value have signatures like

    pub fn alias<S: Into<&'help str>>(mut self, name: S) -> Self {
        self.aliases.push((name.into(), false));
        self
    }

Currently, that looks like

    pub fn alias<S: Into<&'help str>>(mut self, name: S) -> Self {
        self.aliases.push((name.into(), false));
        self
    }

This PR changes it to

    pub fn aliases<I, T>(mut self, names: I) -> Self
    where
        I: IntoIterator<Item = T>,
        T: Into<&'help str>,
    {
        self.aliases
            .extend(names.into_iter().map(|x| (x.into(), false)));
        self
    }

So value_names's T matches value_name's S.

[pksunkara]

Ah, yeah we should not mind the signature being inconsistent between singular and plural apis as long as all kinds of inputs are accepted by the function.

[pksunkara]

Something to keep in mind, https://doc.rust-lang.org/edition-guide/rust-2021/IntoIterator-for-arrays.html

[epage]

I played with it some more and, as expected, received some lifetime complaints (because I am not taking the approach of doing a &AsRef<str>.

We should defer this

• As I said before, this can become trivial when we change our string type and we should just wait until then rather than forcing something to work that is a half measure to then just spend the time to fix it again later.
• In general, I think this ends up being more of a distraction from the current release. We should be focusing more on the work already done rather than continuing new work.