Add option to vertically align enum discriminants.

[moxian]

Add enum_discrim_align_threshold config option, which works similarly to the struct_field_align_threshold one.
If the enum identifiers can be padded with no more than enum_discrim_align_threshold spaces to ensure vertical alignment of the discriminants - do so.

As an example (from #1686):

pub enum ColorimetricIntentImageState {
    SceneColorimetryEstimates = 0x73636F65,
    SceneAppearanceEstimates = 0x73617065,
    FocalPlaneColorimetryEstimates = 0x66706365,
    ReflectionHardcopyOriginalColorimetry = 0x72686F63,
    ReflectionPrintOutputColorimetry = 0x72706F63,
}

now becomes

pub enum ColorimetricIntentImageState {
    SceneColorimetryEstimates             = 0x73636F65,
    SceneAppearanceEstimates              = 0x73617065,
    FocalPlaneColorimetryEstimates        = 0x66706365,
    ReflectionHardcopyOriginalColorimetry = 0x72686F63,
    ReflectionPrintOutputColorimetry      = 0x72706F63,
}

(but note the different alignment of = from the one suggest in that issue. cc @kornelski in case they feel strongly about this)

Fixes #1686
Is part of the larger vertical-alignment project at #1103

[moxian]

I've given this a bit more thought (and read https://www.joelonsoftware.com/2000/04/12/choices/), and I now think that there should only be a single vertical_align_threshold option and not per item type (struct_field_align_threshold, enum_discrim_align_threshold, match_arm_align_threshold etc).

I'm not sure if it's appropriate to unify those (and, necessarily, rename existing struct_field_align_threshold) in this particular PR (I don't think it is), but just leaving this out there in case option name bikeshedding comes to be.

[hazelweakly]

Just out of curiosity, why is there a threshold in the first place? I would've envisioned a vertically_align boolean option that, when enabled, does its best to vertically align everything that makes sense and damn the threshold. Why would a user expect

pub enum ColorimetricIntentImageState {
    SceneColorimetryEstimates = 0x73636F65,
    SceneAppearanceEstimates = 0x73617065,
    FocalPlaneColorimetryEstimates = 0x66706365,
    ReflectionHardcopyOriginalColorimetry = 0x72686F63,
    ReflectionPrintOutputColorimetry = 0x72706F63,
}

to align, but then expect

pub enum ColorimetricIntentImageState {
    SceneColorimetryEstimates = 0x73636F65,
    SceneAppearanceEstimates = 0x73617065,
    FocalPlaneColorimetryEstimatesThisIsAVeryAwkwardlyLongAndSuperAwkwardlyWrittenTypeThatIsFarTooLongToBeReasonable = 0x66706365,
    ReflectionHardcopyOriginalColorimetry = 0x72686F63,
    ReflectionPrintOutputColorimetry = 0x72706F63,
}

to not align? Sure, the pathological edge case has a ridiculous variable name, but even if it was a legitimate 60 characters wide or so, I'd still expect those to be aligned. What are your thoughts?

[nrc]

Thanks for the PR! I do agree with your comment that just having one threshold rather than one per item would be better, but you don't need to change that in this PR.

[nrc]

An alternative would be to align all variants that are less than the threshold, rather than skip the whole enum:

enum TooLong {
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaa = 10,
    A     = 1,
    Bcdef = 2,
}

What do you think?

[moxian]

First of all, I'm afraid I won't be able to get to this PR for a couple of weeks. Apologies.

To address the comments:

Why would a user expect <..> to align but <..> to not align?

Because

enum Foo {
    A                                                                                                                                                                                                                                         = 1,
    Ab,
    Abc                                                                                                                                                                                                                                       = 3,
    Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa = 4,
}

is hard to read. Both because it's hard to correlate variant name with discrim and it's possible to think the variant has no discrim at all (since it's offset so far to the right).

An alternative would be to align all variants that are less than the threshold, rather than skip the whole enum

This sounds like a good idea, but I'm afraid there are weird edge-cases. Consider:

enum TooLong {
    A = 1,
    ABC = 2,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaa = 10,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaB = 11,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBC = 12,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBCD = 13,
}

The "preferred" way to format this is, I would guess:

enum TooLong {
    A   = 1,
    ABC = 2,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaa    = 10,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaB   = 11,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBC  = 12,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBCD = 13,
}

but I'm not sure how to algorithmify that nicely. It would also likely output some weirdness with more
variants of itermediate lengths. Also reordering the fields would definitely make it look bad:

enum TooLong {
    A   = 1,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaa    = 10,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaB   = 11,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBC  = 12,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBCD = 13,
    ABC = 2,
}

So instead of fixing the formatting half-way, I opt to throw the arms up in the air and not touch the formatting at all, falling back to default unaligned layout that is certainly at least fine for all the cases.

[nrc]

On the other hand, I think the following (assuming only the longest variant is too long) looks fine:

enum TooLong {
    A                                               = 1,
    ABC                                             = 2,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaa   = 10,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaB  = 11,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBC = 12,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBCD = 13,
}

and (assuming all the long variants are too long), nor does:

enum TooLong {
    A   = 1,
    ABC = 2,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaa = 10,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaB = 11,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBC = 12,
    ThisOneHasDiscrimAaaaaaaaaaaaaaaaaaaaaaaaaaaaBCD = 13,
}

[moxian]

My apologies for dropping this and letting it sit in limbo for as long as it did. Things happened...

I guess there was a slight misunderstanding in that I wanted to align the discrims if they are "not too far apart", and it would be better to align them "if they aren't too long". This solves nicely the common case, and avoids having to come up with nice solution to format

enum Foo {
  short  = 1,
  short2 = 2,
  mediumlength  = 11,
  mediumlength2 = 12,
  veryveryverylongthing  = 101
  veryveryverylongthing2 = 102
}

the way I have here. Instead it claims this case to be "noncompliant", and either aligning only the shorts or shorts and mediums up to medium (depending on the value).

Anyway, please take another look!

[nrc]

Thanks!