fix(join): always put comma at the end of tuple statement

[mi-caren]

No description provided.

[paolobarbolini]

More context @dodomorandi and why this is necessary: there are cases in which we need to generate a one-element tuple. The current implementation doesn't leave a trailing comma, so what gets generated is (unwrapped{some_number}.{some_other_number}). Rust doesn't see this as a tuple, so the map's value is T, not (T,). This then messes the presenter, which expects the value to always be a tuple.

[dodomorandi]

The base idea is perfect, the only thing that is not convincing me is what happens when or_insert is empty. In that case the expression (,) is being created, but that is not valid.

Do we have any guarantee or_insert is never empty? If not we could just create a branch that creates a token stream with the current expansion if or_insert is not empty and, in the other case, it just creates a unit.

[paolobarbolini]

Should we make some kind of helper for generating tuples that also handles the unit case?

[dodomorandi]

Good idea indeed!

[paolobarbolini]

LGTM

[paolobarbolini]

I'd move this to utils.rs

[dodomorandi]

Extreme nitpick mode on: I would propose a couple of renames and a change of input type:

fn tuple_from_tokenizables<I>(tokenizables: I) -> TokenStream
where
    I: IntoIterator<Item: ToTokens, IntoIter: ExactSizeIterator>,
{
    let tokenizables = tokenizables.into_iter();
    if tokenizables.len() == 0 {
        quote! { () }
    } else {
        quote! { (#(#tokenizables),*,) }
    }
}

tokenizables is not the best name I could think of, if you have a better proposal feel free to go on.

Unfortunately ExactSizeIterator::is_empty is still unstable, the following approach could be possible but I don't think it's worth it.

fn tuple_from_tokenizables<I>(tokenizables: I) -> TokenStream
where
    I: IntoIterator<Item: ToTokens>,
{
    let mut tokenizables = tokenizables.into_iter();
    if let Some(tokenizable) = tokenizables.next() {
        quote! { (#tokenizable, #(#tokenizables),*,) }
    } else {
        quote! { () }
    }
}

Said that, you can also go on with the current implementation. My proposal is useful in case you do not have a slice of tokenizable elements.