[mdoc] Attributes are aggregated and potentially mismatched

[dend]

Related to this issue in .NET docs:
https://github.com/dotnet/docs/issues/1876

The problem revolves around the fact that we are aggregating attributes at reflection time, in frameworks mode. So when we put together Xamarin and .NET DLLs, we get the full list of attributes with no regard for the Applies only to, which causes issues as the one referenced - technically, SmtpClient is deprecated in Xamarin, but not in .NET Framework.

[joelmartinez]

At first thought, this should be resolved in similar fashion to #16, #54 ... by adding a FrameworkAlternate attribute to the <Attribute> tag when an outlier is detected. However, this assumes that the attribute will only be available in a single "other" framework. In this case, we'll probably see this in three separate frameworks (ios, mac, and android?).

Should we make FrameworkAlternate a comma separated value?

      <Attributes>
        <Attribute FrameworkAlternate="ios,mac,android">
          <AttributeName>System.Obsolete("...")</AttributeName>
        </Attribute>
      </Attributes>

The above would indicate that this attribute is only available in those specific frameworks ... and not in any others not named. Thoughts?

[dend]

@joelmartinez I think comma-separating here would make sense. @jonpryor - any thoughts on such a format?

[jonpryor]

I agree that some kind of separator would be appropriate; I'm less clear on what that separator should be. Commas without spaces isn't always the easiest thing to read; perhaps spaces would be sufficient?

    <Attribute FrameworkAlternate="ios mac android">

[joelmartinez]

I'm ok with spaces as separators, and agree on readability ... @dend, are there any framework identifiers currently in use that have spaces?

[dend]

@joelmartinez nope, framework identifiers, by convention, will never use spaces, so I am OK with using that as a separator.

[MartinJohns]

Why not just use commas? That would, as far as I know, align with the rest of the MSBuild configurations, e.g. RuntimeIdentifiers.

[jonpryor]

@MartinJohns: If MSBuild "compatibility" is desired, then , isn't good either, because you can't use , in an MSBuild property value on the command-line:

msbuild /p:FrameworkAlternate=ios,mac,android # NOPE

Specify each property separately, or use a semicolon or comma to separate multiple properties

The , introduces a new property.

This perhaps might not matter, but I find it very useful to be able to specify/override MSBuild properties on the command-line.

Spaces can be used; they just require quoting:

msbuild "/p:FrameworkAlternate=ios mac android" # YUP

Alternatively, : can be used:

msbuild "/p:FrameworkAlternate=ios:mac:android" # YUP

Within the Attribute/@FrameworkAlternate context, I still find spaces more readable than comma , or colon :.

[MartinJohns]

I was mistaken, it's ; for RuntimeIdentifiers, not a comma. It's also a semicolon for PackageTargetFallback and TargetFrameworks. For some cases they also use a singular name when it's only one option, and an additional plural name when there are multiple options - but I dislike this personally very much. See here: https://docs.microsoft.com/en-us/dotnet/articles/core/tools/csproj

What I just want to see avoided is that I always have to look up what separator is used for this particular setting. Is it a semicolon like for RuntimeIdentifiers? Or is it a space like for FrameworkAlterate?

I honestly don't know the state of the ecosystem and how it's done with other settings. I'd just love to see this stuff more aligned and streamlined, and not everyone cooking their own solution.

Personally I consider spaces less readable for cases like this. They're usually just used to split words - not options. To provide options we generally have a special separation character, like the famous semicolon. But this is a very subjective thing.

late edit:
I think the way spaces are used is not very intuitive either, that you have to wrap the entire argument with quotes. I'd expect it to be like /p:FrameworkAlternate="ios mac android" instead of the mentioned way by @jonpryor. With an alternate separator character you don't have this issue in the first place.

[jonpryor]

I wish they hadn't used ; as the delimiter for $(RuntimeIdentifiers). That's a mistake. :-(

[MartinJohns]

Whether that was a mistake or not is certainly subjective and debatable. :-)

Aligning the future of the MSBuild properties is not.

[dend]

To lock this down, are we using spaces as separators?

@jonpryor @joelmartinez @MartinJohns

[MartinJohns]

@dend Personally I consider the use of spaces as separators as further fragmentation of the whole MSBuild landscape, leaving an even worse impressions on the users and generating a worse user experience. But I tried to explain my reasoning behind this earlier already. In the end the decision is up to the team, not me. :-)

[joelmartinez]

To follow up on this ... I can certainly understand and even agree with the rationale to use spaces. However, I think aligning with other products makes more sense in the long run, and since there is prior art using ;, we will move forward with that :)

[joelmartinez]

This issue, for now, is being re-opened. The originally submitted fix was pulled from mdoc's 5.7 release because continued testing kept finding failures in certain scenarios. In the end, rather than continue fighting the technical debt that was causing this fix to be more difficult than it should be ... I'm starting off on some work that I was originally hoping to defer until #258.

While this set of refactorings/changes won't resolve #258, they will be foundational in allowing that implementation ... so this still moves us further down the field. I will follow up the 5.7 release with a point release that includes this fix soon.

/cc @dend, @conceptdev