[oleksandrlazarenko-pi]

Fixes #18. The generated accessors carried only the property description. They now also carry the property URL as a link and, where the metadata exports values, a fixed width table of value and description - in C# (remarks/see href/code), Java (pre block plus a trailing see tag) and Rust (markdown link plus a text fence). Tables cap at 20 rows with an overflow line pointing at the URL, and descriptions are collapsed to one line and shortened at 120 chars. Last of three PRs: the schema change in the property metadata and the extract from the device data system of record in the metadata package are merged. Values have not yet landed in the metadata, so until the first auto-update run this emits the URL block only (124 properties today) - including for properties whose values the extract withholds for being over its maximum, which arrive as ExportValues true with no Values and correctly render without a table. Verified by compiling the builders against the merged FiftyOne.MetaData and pipeline-dotnet projects: 21 render assertions pass, including the issue's IsSmartPhone table byte-for-byte in all three languages. NOTE: PropertyGenerator itself cannot restore on a workstation (FiftyOne.MetaData and both engine packages are on the GitHub Packages feed), so CI is the first full build.

[oleksandrlazarenko-pi]

One thing to decide outside this diff: this repository has no pull request CI (only the monthly copyright workflow), and PropertyGenerator only restores against the private feed, so nothing builds a PR here before merge - the first full build of any change is the next accessor-generation run in a consuming repo's nightly. The builders in this PR were compiled and render-tested externally against the merged FiftyOne.MetaData (22 assertions), but Program.cs and the two generator classes are only covered by being unchanged. A minimal workflow doing a dotnet build of PropertyGenerator with the GitHub Packages feed would close that; happy to add it here or separately.

[oleksandrlazarenko-pi]

Done in the latest commit rather than separately - pull-request.yml now builds both solutions on a PR, using the same ACCESS_TOKEN the monthly copyright workflow already uses for the feed. Validating it locally before pushing caught two things a first CI run would have tripped on: a RuntimeIdentifier cannot be applied at solution level (NETSDK1134), so PropertyGenerator builds at project level with -a x64 like the generation script, and CopyrightUpdater targets net10.0, so both SDKs are installed. With the feed configured locally PropertyGenerator now also builds on a workstation - full project including Program.cs, zero warnings from this PR's files - so the earlier note about nothing building before merge no longer applies either way.

[justadreamer]

Findings from a review of the generated comment output, most severe first. Each is inline with a suggestion where the fix is a single hunk; the Javadoc escaping and the column width cap are each split across more than one suggestion which need applying together.

[justadreamer]

EscapeMarkup covers the markup characters, but not the two sequences that a Javadoc comment cannot survive. Javadoc is a block comment, so a value name or description containing */ ends it early: a description such as Written as /* comment */ in CSS emits * Written as /* comment */ in CSS inside /** ... */, and the rest of the table plus the closing */ become stray tokens - the generated *Data.java does not compile. Separately, Javadoc still reads a leading @token on a comment line as a block tag and {@ as an inline tag even inside <pre>, so a value named @font-face produces unknown tag, which is an error under -Xdoclint and fails the javadoc jar step in the consuming repositories.

C# and Rust use line comments and are unaffected, so this belongs in a Java specific escape rather than in EscapeMarkup. Applied together with the suggestion on JavaClassBuilderBase.cs:

Suggested change

	internal static string EscapeMarkup(string text)
	{
	return (text ?? string.Empty)
	.Replace("&", "&")
	.Replace("<", "&lt;")
	.Replace(">", "&gt;")
	.Replace("\"", "&quot;");
	}
	internal static string EscapeMarkup(string text)
	{
	return (text ?? string.Empty)
	.Replace("&", "&amp;")
	.Replace("<", "&lt;")
	.Replace(">", "&gt;")
	.Replace("\"", "&quot;");
	}
	/// <summary>
	/// Escape a line for a Javadoc comment. Javadoc is a block comment, so
	/// a value carrying "*/" would end it early and leave the rest of the
	/// table as code. A line starting with "@", or carrying "{@", is read
	/// as a tag even inside a &lt;pre&gt; block.
	/// </summary>
	internal static string EscapeJavaDoc(string text)
	{
	var escaped = EscapeMarkup(text)
	.Replace("*/", "*&#47;")
	.Replace("{@", "{&#64;");
	return escaped.StartsWith("@", StringComparison.Ordinal) ?
	"&#64;" + escaped.Substring(1) :
	escaped;
	}

[oleksandrlazarenko-pi]

Taken as suggested - EscapeJavaDoc with the three replacements, kept separate from EscapeMarkup since C# and Rust write line comments and only Java has the block-comment exposure. Applied to the table lines, the description and the URL, since a / inside an href ends the comment just the same. Harness now generates from values named @font-face and descriptions carrying / comment */ and {@code}: the Java output escapes all three and the C# output correctly keeps them raw.

[justadreamer]

Pairs with the EscapeJavaDoc suggestion on PropertyDocumentation.cs: the table lines are the ones carrying metadata text into a block comment.

Suggested change

	yield return indent + " * " +
	PropertyDocumentation.EscapeMarkup(line);
	yield return indent + " * " +
	PropertyDocumentation.EscapeJavaDoc(line);

The description on line 59 has the same exposure and is not escaped at all today, so a */ or a leading @ in a property description breaks the generated Java in the same way. Worth putting through EscapeJavaDoc too.

[oleksandrlazarenko-pi]

Both done - the table lines and the raw description on the line above now go through EscapeJavaDoc. You're right that the description exposure predates this PR; with values arriving it would only have grown.

[justadreamer]

There is a floor on the value column but no ceiling, so one long value sets the width for every row. Shorten caps a name at MaxValueDescriptionLength, which means a 150 character value produces a 122 wide column: every other description starts at column 122 and each comment line runs past 250 characters. The fixed width table stops being readable and the tooltip that shows it stops being useful.

Three suggestions together - this one adds the cap:

Suggested change

	/// <summary>
	/// Minimum width of the value column. Chosen so that the short
	/// enumeration style values which prompted this documentation
	/// (True, False, Unknown, N/A) line up in a readable column.
	/// </summary>
	private const int MinimumValueColumnWidth = 10;
	/// <summary>
	/// Number of spaces between the longest value and the start of the
	/// description column.
	/// </summary>
	private const int ValueColumnPadding = 2;
	/// <summary>
	/// Minimum width of the value column. Chosen so that the short
	/// enumeration style values which prompted this documentation
	/// (True, False, Unknown, N/A) line up in a readable column.
	/// </summary>
	private const int MinimumValueColumnWidth = 10;
	/// <summary>
	/// Widest the value column is allowed to become. Without a ceiling a
	/// single long value pushes every description out to its width, which
	/// is neither a readable table nor a line an IDE tooltip can show.
	/// </summary>
	private const int MaximumValueColumnWidth = 40;
	/// <summary>
	/// Number of spaces between the longest value and the start of the
	/// description column.
	/// </summary>
	private const int ValueColumnPadding = 2;

[oleksandrlazarenko-pi]

All three suggestions taken together - the 40 ceiling, the clamp, and the separator for the outlier. Pinned by two checks: a 200 character value alongside a short one leaves the short row padded to column 40, and the long value itself gets its own two space separator before its description.

[justadreamer]

Clamp to the cap:

Suggested change

	var columnWidth = Math.Max(
	documented.Max(value => value.Name.Length) + ValueColumnPadding,
	MinimumValueColumnWidth);
	var columnWidth = Math.Min(
	Math.Max(
	documented.Max(value => value.Name.Length) +
	ValueColumnPadding,
	MinimumValueColumnWidth),
	MaximumValueColumnWidth);

[justadreamer]

With the column capped, PadRight is a no-op for a value wider than the column and the description would butt straight up against it, so the outlier needs its own separator:

Suggested change

	private static string BuildRow(DocumentedValue value, int columnWidth)
	{
	var description = Shorten(value.Description);
	return description.Length == 0 ?
	value.Name :
	value.Name.PadRight(columnWidth) + description;
	}
	private static string BuildRow(DocumentedValue value, int columnWidth)
	{
	var description = Shorten(value.Description);
	if (description.Length == 0)
	{
	return value.Name;
	}
	var padded = value.Name.PadRight(columnWidth);
	return padded.Length > columnWidth ?
	padded + new string(' ', ValueColumnPadding) + description :
	padded + description;
	}

[justadreamer]

This repository is public and the trigger is a plain pull_request, so secrets.ACCESS_TOKEN is empty for a pull request raised from a fork. The feed is then added with a blank password and the restore of FiftyOne.MetaData fails with 401, making the check red on every fork pull request whatever it contains. Skipping is more honest than failing:

Suggested change

	jobs:
	Build:
	runs-on: ubuntu-latest
	jobs:
	Build:
	# The restore needs the organisation's package feed, and a pull request
	# from a fork cannot read the secret which authenticates it.
	if: github.event.pull_request.head.repo.full_name == github.repository
	runs-on: ubuntu-latest

[oleksandrlazarenko-pi]

Taken as suggested. Agreed that a guaranteed-red check on every fork PR is worse than no check - it trains people to ignore the signal.

[justadreamer]

FromMetaData guards Values == null a few lines up, this twin does not, so a GetValues() returning null throws and aborts generation rather than emitting an accessor with no table. The engine backed builders are not reachable from Program today, so this is consistency rather than a live failure:

Suggested change

	var values = new List<DocumentedValue>();
	foreach (var value in property.GetValues())
	var values = new List<DocumentedValue>();
	foreach (var value in property.GetValues() ??
	Enumerable.Empty<IValueMetaData>())

[oleksandrlazarenko-pi]

Guarded as suggested, matching the FromMetaData twin.

[justadreamer]

A value description containing three backticks closes this fence early and the remaining rows are parsed as rustdoc markdown, which can trip lints the file's #![allow] does not cover - it allows bare_urls and invalid_html_tags only. Rendering only, /// cannot break the compile. Cheapest fix is to pick a fence longer than the longest backtick run in the table, rather than a fixed three.

Same class of thing on the link above: the pointy bracket destination guards against ( and ), but a literal > in the URL still ends it early.

[oleksandrlazarenko-pi]

Fence is now computed as one longer than the longest backtick run in the table, minimum three, and a literal > in a URL is percent encoded as %3E so it cannot close the pointy bracket destination while the link keeps working. Both covered in the harness.

[justadreamer]

This three line assembly - fetch the URL, build the table from it, branch on emptiness - is repeated verbatim in JavaClassBuilderBase.GetJavaDocBody and RustClassBuilderBase.PropertyDoc; only the wrapping differs. A helper on ClassBuilderBase returning the url and the table together would keep the three emitters in lockstep as the documentation grows.

Related: for C# and Java the table is built twice per property, once for the interface file and once for the class file. Free for metadata properties, but FromEngine walks and disposes every value of the property on each call.

[oleksandrlazarenko-pi]

Extracted to GetPropertyDocumentation on ClassBuilderBase, returning the URL and table together and memoised per property - so the three emitters share one assembly path and the interface and class files reuse one build, which also means FromEngine walks and disposes an engine's values once rather than per file.