Pr397 - exchange set specification update - #406
Open
Cairn23 wants to merge 29 commits into
Open
Conversation
Whilst testing found that a number of items were not geeting the data from the catalogue file dur to the scehme versoins being incorrect and the element tags were not correct
…ins. Error was in the PT_Locale
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the exchange set catalogue model and parsing to align more closely with the evolving S-100 Exchange Catalogue schema, and propagates stronger date/enum types into the Viewer’s exchange set header display and related tests.
Changes:
- Switched multiple catalogue/viewer “date” fields from
stringtoDateOnly?and updated Viewer tests accordingly. - Expanded
EncDotNet.S100.ExchangeSetscatalogue model (new enums/types likePurpose,NavigationPurpose,TemporalExtent,PT_Locale, etc.) and updatedExchangeCatalogueReaderto parse additional elements. - Updated
.gitignoreto exclude additional Visual Studio artifacts.
Reviewed changes
Copilot reviewed 19 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/EncDotNet.S100.Viewer.Tests/ExchangeSetServiceLoaderTests.cs | Updates assertions to use DateOnly for exchange set header issue date. |
| tests/EncDotNet.S100.Viewer.Tests/DatasetsViewModelExchangeSetHeaderTests.cs | Updates test calls/assertions for RegisterExchangeSetHeader to use DateOnly. |
| tests/EncDotNet.S100.Viewer.Tests/DatasetsPanelTabsTests.cs | Updates exchange set header registration to pass DateOnly. |
| tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs | Updates expectations for typed purpose/date and the new DefaultLocale object. |
| src/EncDotNet.S100.Viewer/ViewModels/ExchangeSetHeader.cs | Changes IssueDate to DateOnly? and threads through metadata summary generation. |
| src/EncDotNet.S100.Viewer/ViewModels/DatasetsViewModel.cs | Updates RegisterExchangeSetHeader signature to accept DateOnly?. |
| src/EncDotNet.S100.Viewer/Services/ExchangeSetService.cs | Updates “latest issue date” resolution to work with DateOnly?. |
| src/EncDotNet.S100.ExchangeSets/TemporalExtent.cs | Adds a temporal extent model type for catalogue parsing. |
| src/EncDotNet.S100.ExchangeSets/Purpose.cs | Adds typed purpose enum for dataset discovery metadata. |
| src/EncDotNet.S100.ExchangeSets/PT_Locale.cs | Adds a locale model type used by catalogue/default locale parsing. |
| src/EncDotNet.S100.ExchangeSets/ProductSpecification.cs | Types product specification date and compliancy category. |
| src/EncDotNet.S100.ExchangeSets/NavigationPurpose.cs | Adds typed navigation purpose enum for dataset discovery metadata. |
| src/EncDotNet.S100.ExchangeSets/MaintenanceInformation.cs | Adds maintenance information model + frequency enum for catalogue parsing. |
| src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs | Expands catalogue parsing for new typed fields/locales/coverage/maintenance, plus date/time parsing helpers. |
| src/EncDotNet.S100.ExchangeSets/ExchangeCatalogue.cs | Replaces separate default-locale fields with PT_Locale + other locales list. |
| src/EncDotNet.S100.ExchangeSets/DatasetDiscoveryMetadata.cs | Adds new parsed fields and converts several fields to enums/date/time typed properties. |
| src/EncDotNet.S100.ExchangeSets/DataCoverage.cs | Extends data coverage model with additional fields (scales/resolution/temporal extent). |
| src/EncDotNet.S100.ExchangeSets/CompliancyCategory.cs | Adds compliancy category enum used by product specification parsing. |
| src/EncDotNet.S100.ExchangeSets/CatalogueDiscoveryMetadata.cs | Converts issue date to DateOnly? and replaces locale fields with PT_Locale + other locales. |
| .gitignore | Adds ignores for additional VS solution artifacts. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| .Elements(lan + "CountryCode") | ||
| .FirstOrDefault(); | ||
|
|
||
| var country = (string?)langCode?.Attribute("codeListValue"); |
| var charEncode = moreLocal? | ||
| .Elements(lan + "characterEncoding") | ||
| .FirstOrDefault()? | ||
| .Elements(lan + "MD_CharacterSetCode ") |
| .Elements(lan + "MD_CharacterSetCode ") | ||
| .FirstOrDefault(); | ||
|
|
||
| var encoding = (string?)langCode?.Attribute("codeListValue"); |
Comment on lines
287
to
294
| return new DataCoverage | ||
| { | ||
| BoundingPolygon = element.Element(xc + "boundingPolygon")?.ToString(), | ||
| MaximumDisplayScale = int.TryParse(maxStr, CultureInfo.InvariantCulture, out var max) ? max : null, | ||
| MinimumDisplayScale = int.TryParse(minStr, CultureInfo.InvariantCulture, out var min) ? min : null, | ||
| OptimumDisplayScale = int.TryParse(optStr, CultureInfo.InvariantCulture, out var opt) ? opt : null, | ||
| ApproximateGridResolution = float.TryParse(resStr, CultureInfo.InvariantCulture, out var res) ? res : null, | ||
| }; |
Comment on lines
+109
to
+118
| Purpose? purpose = null; | ||
| string? purposeStr = (string?)element.Element(xc + "purpose"); | ||
| if (purposeStr != null) | ||
| purpose = (Purpose)Enum.Parse(typeof(Purpose), purposeStr); | ||
|
|
||
|
|
||
| NavigationPurpose? navigationPurpose = null; | ||
| string? naxPurposeStr = (string?)element.Element(xc + "navigationPurpose"); | ||
| if (naxPurposeStr != null) | ||
| navigationPurpose = (NavigationPurpose)Enum.Parse(typeof(NavigationPurpose), naxPurposeStr); |
| if (!string.IsNullOrWhiteSpace(issueDate)) | ||
| if (issueDate != null) | ||
| { | ||
| parts.Add(string.Format(Strings.Pane_ExchangeSetHeader_Issued, issueDate)); |
Comment on lines
267
to
273
| [Fact] | ||
| public void DefaultLocale_IsNull() | ||
| { | ||
| var catalogue = ReadTestCatalogue(); | ||
|
|
||
| Assert.Null(catalogue.DefaultLocaleLanguage); | ||
| Assert.Null(catalogue.DefaultLocaleCharacterEncoding); | ||
| Assert.Null(catalogue.DefaultLocale); | ||
| } |
Comment on lines
+47
to
+50
| /// <summary>Catalogue-derived issue date string (the latest | ||
| /// <c>DatasetDiscoveryMetadata.IssueDate</c> across the set), or | ||
| /// <c>null</c> if unknown.</summary> | ||
| public string? IssueDate { get; } | ||
| public DateOnly? IssueDate { get; } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+562
to
+564
| var country = (string?)langCode?.Attribute("codeListValue"); | ||
| if (country == null) | ||
| country = ""; |
Comment on lines
+566
to
+574
| var charEncode = moreLocal? | ||
| .Elements(lan + "characterEncoding") | ||
| .FirstOrDefault()? | ||
| .Elements(lan + "MD_CharacterSetCode ") | ||
| .FirstOrDefault(); | ||
|
|
||
| var encoding = (string?)langCode?.Attribute("codeListValue"); | ||
| if (encoding == null) | ||
| encoding = ""; |
Comment on lines
285
to
294
| TemporalExtent? temporalExtent = ReadTempoalExtent(element, xc); | ||
|
|
||
| return new DataCoverage | ||
| { | ||
| BoundingPolygon = element.Element(xc + "boundingPolygon")?.ToString(), | ||
| MaximumDisplayScale = int.TryParse(maxStr, CultureInfo.InvariantCulture, out var max) ? max : null, | ||
| MinimumDisplayScale = int.TryParse(minStr, CultureInfo.InvariantCulture, out var min) ? min : null, | ||
| OptimumDisplayScale = int.TryParse(optStr, CultureInfo.InvariantCulture, out var opt) ? opt : null, | ||
| ApproximateGridResolution = float.TryParse(resStr, CultureInfo.InvariantCulture, out var res) ? res : null, | ||
| }; |
Comment on lines
+92
to
+95
| CompliancyCategory? comp = null; | ||
| string? compStr = (string?)element.Element(xc + "compliancyCategory"); | ||
| if (compStr != null) | ||
| comp = (CompliancyCategory)Enum.Parse(typeof(CompliancyCategory), compStr); |
Comment on lines
703
to
707
| /// date strings sort correctly under ordinal comparison, so no | ||
| /// parsing is needed for the common case. | ||
| /// </summary> | ||
| private static string? ResolveLatestIssueDate( | ||
| private static DateOnly? ResolveLatestIssueDate( | ||
| IReadOnlyList<DatasetDiscoveryMetadata> datasets) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+1
to
+10
| | ||
| namespace EncDotNet.S100.ExchangeSets | ||
| { | ||
| public enum NavigationPurpose | ||
| { | ||
| port = 1, | ||
| transit = 2, | ||
| overview = 3 | ||
| } | ||
| } |
Comment on lines
+496
to
+500
| private static PT_Locale? ReadPTLocale(XElement? localeElement, XNamespace lan) | ||
| { | ||
| if (localeElement is null) return null; | ||
|
|
||
| XElement? moreLocal = localeElement.Element(lan + "PT_Locale"); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
287
to
295
| return new DataCoverage | ||
| { | ||
| BoundingPolygon = element.Element(xc + "boundingPolygon")?.ToString(), | ||
| MaximumDisplayScale = int.TryParse(maxStr, CultureInfo.InvariantCulture, out var max) ? max : null, | ||
| MinimumDisplayScale = int.TryParse(minStr, CultureInfo.InvariantCulture, out var min) ? min : null, | ||
| OptimumDisplayScale = int.TryParse(optStr, CultureInfo.InvariantCulture, out var opt) ? opt : null, | ||
| ApproximateGridResolution = float.TryParse(resStr, CultureInfo.InvariantCulture, out var res) ? res : null, | ||
| }; | ||
| } |
Comment on lines
+562
to
+574
| var country = (string?)langCode?.Attribute("codeListValue"); | ||
| if (country == null) | ||
| country = ""; | ||
|
|
||
| var charEncode = moreLocal? | ||
| .Elements(lan + "characterEncoding") | ||
| .FirstOrDefault()? | ||
| .Elements(lan + "MD_CharacterSetCode ") | ||
| .FirstOrDefault(); | ||
|
|
||
| var encoding = (string?)langCode?.Attribute("codeListValue"); | ||
| if (encoding == null) | ||
| encoding = ""; |
Comment on lines
+92
to
+95
| CompliancyCategory? comp = null; | ||
| string? compStr = (string?)element.Element(xc + "compliancyCategory"); | ||
| if (compStr != null) | ||
| comp = (CompliancyCategory)Enum.Parse(typeof(CompliancyCategory), compStr); |
Comment on lines
+109
to
+118
| Purpose? purpose = null; | ||
| string? purposeStr = (string?)element.Element(xc + "purpose"); | ||
| if (purposeStr != null) | ||
| purpose = (Purpose)Enum.Parse(typeof(Purpose), purposeStr); | ||
|
|
||
|
|
||
| NavigationPurpose? navigationPurpose = null; | ||
| string? naxPurposeStr = (string?)element.Element(xc + "navigationPurpose"); | ||
| if (naxPurposeStr != null) | ||
| navigationPurpose = (NavigationPurpose)Enum.Parse(typeof(NavigationPurpose), naxPurposeStr); |
| DefaultLocale = ReadPTLocale(element.Element(xc + "defaultLocale"), lan), | ||
| OtherLocales = element.Elements(xc + "otherLocale").Select(e => ReadPTLocales(e, lan)).ToList(), | ||
| MetadataDateStamp = ParseDate((string?)element.Element(xc + "metadataDateStamp")), | ||
| ReplaceData = ParseBool(element, "replaceData", xc), |
Comment on lines
+47
to
+50
| /// <summary>Catalogue-derived issue date string (the latest | ||
| /// <c>DatasetDiscoveryMetadata.IssueDate</c> across the set), or | ||
| /// <c>null</c> if unknown.</summary> | ||
| public string? IssueDate { get; } | ||
| public DateOnly? IssueDate { get; } |
Comment on lines
+192
to
195
| if (issueDate != null) | ||
| { | ||
| parts.Add(string.Format(Strings.Pane_ExchangeSetHeader_Issued, issueDate)); | ||
| } |
Comment on lines
699
to
+706
| /// <summary> | ||
| /// Returns the lexically-greatest non-null | ||
| /// <see cref="DatasetDiscoveryMetadata.IssueDate"/> across the | ||
| /// catalogue, or <c>null</c> if no dataset declared one. ISO-8601 | ||
| /// date strings sort correctly under ordinal comparison, so no | ||
| /// parsing is needed for the common case. | ||
| /// </summary> | ||
| private static string? ResolveLatestIssueDate( | ||
| private static DateOnly? ResolveLatestIssueDate( |
Author
|
Fixed review comments and merged |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (7)
tests/EncDotNet.S100.Viewer.Tests/DatasetsViewModelExchangeSetHeaderTests.cs:63
- Same here: prefer DateOnly.ParseExact(..., InvariantCulture) for a fixed-format literal so the assertion doesn’t depend on CurrentCulture parsing rules.
Assert.Equal("ACME", header.Producer);
Assert.Equal(DateOnly.Parse("2024-05-01"), header.IssueDate);
Assert.Equal(7, header.DatasetCount);
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:543
- ReadPTLocale currently turns a missing/empty lan:LanguageCode into an empty string (""), which makes it impossible for callers to distinguish “missing” vs “present but empty” and can lead to blank locale labels. Since this method already returns null for a missing element, it’s more consistent to return null when the required language code is absent as well.
var lang = (string?)langCode?.Attribute("codeListValue");
if (lang == null)
lang = "";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:562
- ReadPTLocale also converts a missing/empty lan:MD_CharacterSetCode into "". For consistency (and to avoid constructing PT_Locale instances with effectively-unknown required fields), consider treating missing/empty characterEncoding the same way as a missing locale element and returning null.
var encoding = (string?)charEncode?.Attribute("codeListValue");
if (encoding == null)
encoding = "";
tests/EncDotNet.S100.Viewer.Tests/ExchangeSetServiceLoaderTests.cs:118
- These tests use DateOnly.Parse with an ISO-like string, but DateOnly.Parse is culture-sensitive. To keep tests stable under non-default cultures, prefer ParseExact with InvariantCulture for fixed-format literals.
Assert.Equal(DateOnly.Parse("2026-01-12"), header.IssueDate);
tests/EncDotNet.S100.Viewer.Tests/DatasetsViewModelExchangeSetHeaderTests.cs:58
- DateOnly.Parse is culture-sensitive; for a fixed yyyy-MM-dd literal, ParseExact with InvariantCulture keeps the test deterministic across locales.
This issue also appears on line 61 of the same file.
var header = vm.RegisterExchangeSetHeader(
src, "/tmp/eset", "ACME", DateOnly.Parse("2024-05-01"), 7, _ => { });
tests/EncDotNet.S100.Viewer.Tests/DatasetsPanelTabsTests.cs:187
- This DateOnly.Parse call is culture-sensitive. Using ParseExact("yyyy-MM-dd", InvariantCulture) avoids locale-dependent parsing in test runs.
var header = vm.RegisterExchangeSetHeader(src, "/a", "ACME", DateOnly.Parse("2024-01-01"), 1, _ => { });
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:277
- DateOnly.Parse is culture-sensitive; these tests use a fixed yyyy-MM-dd literal, so ParseExact with InvariantCulture is safer and keeps the assertion deterministic.
Assert.Equal(DateOnly.Parse("2023-01-16"), dataset.IssueDate);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (9)
src/EncDotNet.S100.Viewer/Services/ExchangeSetService.cs:1135
- The XML doc comment here is grammatically awkward ("Returns the Date of the latest issue date") and doesn't clearly state the "non-null" condition. Rewording it makes the intent clearer for future maintainers.
/// <summary>
/// Returns the Date of the latest issue date among the supplied datasets, or <c>null</c>
/// </summary>
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:285
- There are a few spacing inconsistencies here (spaces before closing parens) that are likely to be flagged/rewritten by formatting verification.
Assert.False(dataset.DataProtection );
Assert.Null(dataset.ProtectionScheme);
Assert.Equal(EncDotNet.S100.ExchangeSets.DigitalSignatureAlgorithm.DSA, dataset.DigitalSignatureAlgorithm );
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:292
- More assertions in this block have spacing inside parentheses that will likely be rewritten by
dotnet format whitespace, leading to formatting verification failures.
Assert.False( dataset.Copyright);
Assert.Null(dataset.Classification);
Assert.True(dataset.NotForNavigation);
Assert.Null(dataset.SpecificUsage);
Assert.Null(dataset.UpdateNumber);
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:294
- Remove the stray whitespace inside the assertion call; it will likely be rewritten by formatting verification.
Assert.Null(dataset.ReferenceId );
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:304
- These assertions also contain extra whitespace inside parentheses; cleaning this up avoids
dotnet formatverification noise/failures.
Assert.Null(dataset.Comment );
Assert.Null(dataset.DefaultLocale);
Assert.Empty(dataset.OtherLocales);
Assert.Null(dataset.MetadataPointOfContact );
Assert.Null(dataset.MetadataDateStamp );
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:283
- Extra whitespace inside these assertions is likely to be rewritten by
dotnet format whitespace; with the repo's format verification in CI this can fail the build due to a dirty formatting diff.
This issue also appears in the following locations of the same file:
- line 283
- line 288
- line 294
- line 300
Assert.Null( dataset.FilePath);
Assert.Null(dataset.Description);
Assert.Equal("urn:mrn:iho:hash:sha256:76c743c91679c9220947cd8c4940a2922056648cfe0b3ebb6665af2a553e5ac3", dataset.DatasetId);
Assert.False(dataset.CompressionFlag);
Assert.False(dataset.DataProtection );
src/EncDotNet.S100.ExchangeSets/PT_Locale.cs:3
- The public type name
PT_Localeis the only type in the repo that uses an underscore in a C# type name, which makes the public API inconsistent with the rest of the codebase’s PascalCase naming. Consider renaming toPtLocale(or similar) and mapping it to the ISO element name in the reader instead.
public sealed class PT_Locale
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogue.cs:13
- These property changes are a breaking public API change (replacing the previous DefaultLocaleLanguage/DefaultLocaleCharacterEncoding string properties with a new PT_Locale model). If this is intentional, it should be called out in the PR description/release notes (or consider retaining the old properties as [Obsolete] shims for a transition).
public PT_Locale? DefaultLocale { get; init; }
public IReadOnlyList<PT_Locale> OtherLocales { get; init; } = [];
EncDotNet.S100.slnx:69
- This change disables building
EncDotNet.S100.Rendering.Scene.Testsfor all Debug solution configurations. If this wasn't intentional, it can hide compile errors locally and make Debug builds inconsistent with CI/Release builds. Consider keeping the project enabled (or documenting why it's excluded).
<Project Path="tests/EncDotNet.S100.Rendering.Scene.Tests/EncDotNet.S100.Rendering.Scene.Tests.csproj">
<Build Solution="Debug|*" Project="false" />
</Project>
….com/Cairn23/EncDotNet.S100 into PR-Exchange-Set-specification-update
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (11)
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:298
- Another placeholder commented-out assert is left in the test. It should be removed or replaced with a real assertion so the test expresses only actionable expectations.
Assert.Null(dataset.TemporalExtent);
// Assert.Equal({EncDotNet.S100.ExchangeSets.ProductSpecification}, dataset.ProductSpecification);
Assert.Equal("AA00", dataset.ProducingAgency);
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:34
- Changing the fallback
lannamespace from.../lan/2.0to.../lan/1.0alters behavior for catalogues that omit thelan:prefix. To preserve backward compatibility, consider probing for both versions when the prefix isn’t declared, instead of hard-coding a single fallback.
XNamespace xc = root.Name.Namespace;
XNamespace lan = root.GetNamespaceOfPrefix("lan") ?? "http://standards.iso.org/iso/19115/-3/lan/1.0";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:595
PtLocale.Countryis nullable, andReadPTLocaleleaves it asnullwhen absent, butReadPTLocalesforces missingCountryCodeto "". This inconsistency makes it harder for consumers to distinguish “missing” vs “present but empty”.
if (country == null)
country = "";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:568
- Minor formatting issue:
CharacterEncoding =encodingis missing a space and will fail the repo’s format verification ifdotnet formatisn’t run. (It also makes the initializer inconsistent with the rest of the file.)
{
Language = lang,
Country = country,
CharacterEncoding =encoding,
};
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:287
- This placeholder commented-out assert (
Assert.Equal({…}, …)) should be removed (or replaced with a real assertion) to keep the test suite clean and avoid confusion about expected behavior.
This issue also appears on line 296 of the same file.
Assert.Equal(EncDotNet.S100.ExchangeSets.DigitalSignatureAlgorithm.DSA, dataset.DigitalSignatureAlgorithm);
// Assert.Equal({EncDotNet.S100.ExchangeSets.DigitalSignatureValue}, dataset.DigitalSignatureValue );
Assert.Null(dataset.ExpectedHash);
src/EncDotNet.S100.ExchangeSets/TemporalExtent.cs:9
- This introduces a new public type without XML documentation. The repo’s guidelines require XML doc comments for public APIs (types and members).
public sealed class TemporalExtent
{
public DateTime? TimeInstantBegin { get; init; }
public DateTime? TimeInstantEnd { get; init; }
}
src/EncDotNet.S100.ExchangeSets/Purpose.cs:12
- This new public enum is missing XML doc comments for the enum and its members, which is required for public APIs in this repo.
public enum Purpose
{
NewDataset = 1,
NewEdition = 2,
Update = 3,
Reissue = 4,
Cancellation = 5,
Delta = 6,
}
src/EncDotNet.S100.ExchangeSets/NavigationPurpose.cs:9
- This new public enum is missing required XML documentation (enum + members).
public enum NavigationPurpose
{
Port = 1,
Transit = 2,
Overview = 3
}
src/EncDotNet.S100.ExchangeSets/CompliancyCategory.cs:11
- This new public enum is missing XML documentation for the enum and its members (required for public APIs).
public enum CompliancyCategory
{
Category1 = 1,
Category2 = 2,
Category3 = 3,
Category4 = 4,
}
src/EncDotNet.S100.ExchangeSets/PtLocale.cs:10
- This new public type (and its public properties) is missing XML documentation, which is required for public APIs in this repo.
public sealed class PtLocale
{
public required string Language { get; init; }
public string? Country { get; init; }
public required string CharacterEncoding { get; init; }
}
src/EncDotNet.S100.ExchangeSets/MaintenanceInformation.cs:18
- This file introduces new public API surface (enum + class) without XML doc comments, and it also starts with extra blank lines. Public APIs in this repo should be documented, and the leading whitespace may trip format checks.
public enum MaintenanceFrequencyCode
{
AsNeeded = 1,
Irregular = 2
}
public sealed class MaintenanceInformation
{
public MaintenanceFrequencyCode? MaintenanceAndUpdateFrequency { get; init; }
public DateOnly? MaintenanceDate { get; init; }
public string? UserDefinedMaintenanceFrequency { get; init; }
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:584
ReadPTLocalesalso coerces missingLanguageCode/MD_CharacterSetCodeto empty strings, but unlikeReadPTLocaleit returns a non-nullPtLocale(so callers can’t represent an invalid/unknown otherLocale cleanly). Consider aligning the two methods by returningPtLocale?here as well and filtering out nulls at the call sites (or makePtLocale.Language/CharacterEncodingnullable if the spec allows them to be omitted).
var lang = (string?)langCode?.Attribute("codeListValue");
if (lang == null)
lang = "";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:562
ReadPTLocalecurrently coerces missingLanguageCode/MD_CharacterSetCodeto empty strings. That makes it hard for consumers to distinguish “missing locale” from a valid-but-empty value, and it can silently create aPtLocaleeven when the catalogue has an incompletedefaultLocaleblock. Prefer treating missing language/encoding as “no locale” and returningnullin that case.
This issue also appears on line 581 of the same file.
var lang = (string?)langCode?.Attribute("codeListValue");
if (lang == null)
lang = "";
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (13)
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:124
ReadProductSpecificationuses the non-genericEnum.TryParse(Type, ...)overload, which is more verbose and boxes the result. Using the generic overload is simpler and avoids the cast/boxing.
CompliancyCategory? comp = null;
string? compStr = (string?)element.Element(xc + "compliancyCategory");
if (compStr != null && Enum.TryParse(typeof(CompliancyCategory), compStr, ignoreCase: true, out var parsedComp))
comp = (CompliancyCategory)parsedComp;
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:542
ReadPTLocalecurrently substitutes empty strings when the language code is missing. That makes it impossible to distinguish "missing" from a real value and can propagate invalid locale data. Since this method already returns nullable, prefer returningnullwhen the locale is incomplete.
var lang = (string?)langCode?.Attribute("codeListValue");
if (lang == null)
lang = "";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:561
ReadPTLocalecurrently substitutes empty strings when the character encoding is missing, which can leak invalid data into the object model. Prefer returningnullfor incomplete locales (or throwing) instead of inventing sentinel values.
var encoding = (string?)charEncode?.Attribute("codeListValue");
if (encoding == null)
encoding = "";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:603
ReadPTLocales(used forotherLocale) substitutes an empty string when the character encoding is missing. That creates aPtLocalewith invalid required fields; prefer throwing anXmlExceptionso invalid catalogues fail deterministically.
var encoding = (string?)charEncode?.Attribute("codeListValue");
if (encoding == null)
encoding = "";
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:620
- Minor wording: the comment reads "Permit dates" (and similarly below for times). This looks like a typo and should be "Permitted".
// Permit dates are xs:date and may carry a trailing 'Z' (e.g. 2018-03-20Z).
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:635
- Minor wording: this comment reads "Permit times"; it looks like it should be "Permitted times".
// Permit times are xs:time and may carry a trailing 'Z' (e.g. 13:45:30Z).
src/EncDotNet.S100.Viewer/ViewModels/ExchangeSetHeader.cs:48
IssueDatewas changed from an ISO-8601 string toDateOnly?. In the UI,DatasetsView.axamlbindsTextdirectly toInspectedExchangeSet.IssueDate, so this will now render using the current UI culture (e.g.MM/dd/yyyy) rather than the previous stableyyyy-MM-ddstring. Consider exposing a formatted string property (or adding a converter) and binding the view to that to avoid a localization-dependent regression.
/// <summary>Catalogue-derived issue date (the latest
/// <c>DatasetDiscoveryMetadata.IssueDate</c> across the set), or
/// <c>null</c> if unknown.</summary>
public DateOnly? IssueDate { get; }
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:584
ReadPTLocales(used forotherLocale) also substitutes an empty string when the language code is missing. SinceotherLocaleentries are expected to be valid/complete, consider failing fast with anXmlExceptionrather than creating aPtLocalewith invalid required fields.
This issue also appears on line 601 of the same file.
var lang = (string?)langCode?.Attribute("codeListValue");
if (lang == null)
lang = "";
src/EncDotNet.S100.ExchangeSets/TemporalExtent.cs:2
- Leading blank lines at the start of the file are likely to be removed by
dotnet format whitespaceand can cause the CI "Format check" job to fail. Remove the initial blank line so the file starts with the namespace declaration.
namespace EncDotNet.S100.ExchangeSets;
src/EncDotNet.S100.ExchangeSets/Purpose.cs:2
- Leading blank line at the start of the file will likely be normalized away by formatting checks; remove it so the file begins with the namespace declaration.
namespace EncDotNet.S100.ExchangeSets;
src/EncDotNet.S100.ExchangeSets/NavigationPurpose.cs:2
- Leading blank line at the start of the file will likely be normalized away by formatting checks; remove it so the file begins with the namespace declaration.
namespace EncDotNet.S100.ExchangeSets;
src/EncDotNet.S100.ExchangeSets/MaintenanceInformation.cs:2
- Leading blank line at the start of the file will likely be normalized away by formatting checks; remove it so the file begins with the namespace declaration.
namespace EncDotNet.S100.ExchangeSets;
src/EncDotNet.S100.ExchangeSets/CompliancyCategory.cs:3
- This file starts with multiple blank lines before the namespace declaration; whitespace formatting checks typically remove these and may fail CI if the repo enforces
dotnet format whitespace. Remove the leading blank lines.
namespace EncDotNet.S100.ExchangeSets;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
tests/EncDotNet.S100.Viewer.Tests/DatasetsViewModelExchangeSetHeaderTests.cs:55
DateOnly.Parse(...)depends on the current culture; for a fixed expected date in a unit test, prefernew DateOnly(y, m, d)to keep the test culture-invariant.
Assert.Equal(DateOnly.Parse("2024-05-01"), header.IssueDate);
tests/EncDotNet.S100.Viewer.Tests/ExchangeSetServiceLoaderTests.cs:229
DateOnly.Parse(...)depends on the current culture; for a fixed expected date in a unit test, prefernew DateOnly(y, m, d)to keep the test culture-invariant.
Assert.Equal(DateOnly.Parse("2026-01-12"), header.IssueDate);
tests/EncDotNet.S100.Viewer.Tests/DatasetsViewModelExchangeSetHeaderTests.cs:50
DateOnly.Parse(...)depends on the current culture; for a fixed date constant in a unit test, prefernew DateOnly(y, m, d)to keep the test culture-invariant.
This issue also appears on line 55 of the same file.
src, "/tmp/eset", "ACME", DateOnly.Parse("2024-05-01"), 7, _ => { });
tests/EncDotNet.S100.Viewer.Tests/DatasetsPanelTabsTests.cs:180
DateOnly.Parse(...)depends on the current culture; for a fixed date constant in a unit test, prefernew DateOnly(y, m, d)to keep the test culture-invariant.
var header = vm.RegisterExchangeSetHeader(src, "/a", "ACME", DateOnly.Parse("2024-01-01"), 1, _ => { });
tests/EncDotNet.S100.ExchangeSets.Tests/ExchangeCatalogueReaderTests.cs:277
DateOnly.Parse(...)depends on the current culture; for a fixed expected date in a unit test, prefernew DateOnly(y, m, d)to keep the test culture-invariant.
Assert.Equal(DateOnly.Parse("2023-01-16"), dataset.IssueDate);
src/EncDotNet.S100.ExchangeSets/PtLocale.cs:12
requirednon-nullable locale fields make it hard to represent “unknown” values (and the current reader code compensates by using empty strings). Consider making these nullable (and droppingrequired) so missing language/encoding remain distinguishable from a real but empty value.
public required string Language { get; init; }
public string? Country { get; init; }
public required string CharacterEncoding { get; init; }
Comment on lines
+630
to
+635
| var sigEl = wrapper.Element(S100SE + "resourceMaintenance"); | ||
| if (sigEl is null) return null; | ||
|
|
||
| var mfreq = (string?)sigEl.Attribute("maintenanceAndUpdateFrequency"); | ||
| var date = (string?)sigEl.Attribute("maintenanceDate"); | ||
| var ufreq = (string?)sigEl.Attribute("userDefinedMaintenanceFrequency"); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:312
defaultLocaleElis declared but never used in ReadCatalogueDiscovery, which will fail the build because warnings are treated as errors. Use the cached element (or remove the variable).
private static CatalogueDiscoveryMetadata ReadCatalogueDiscovery(XElement element, XNamespace xc, XNamespace lan)
{
var defaultLocaleEl = element.Element(xc + "defaultLocale");
var digitalSignatures = ReadDigitalSignatures(element, xc);
return new CatalogueDiscoveryMetadata
{
FileName = (string)element.Element(xc + "fileName")!,
FilePath = (string?)element.Element(xc + "filePath"),
Purpose = (string?)element.Element(xc + "purpose"),
EditionNumber = ParseInt(element, "editionNumber", xc),
Scope = (string?)element.Element(xc + "scope"),
VersionNumber = (string?)element.Element(xc + "versionNumber"),
IssueDate = ParseDate((string?)element.Element(xc + "issueDate")),
ProductSpecification = ReadProductSpecification(element.Element(xc + "productSpecification"), xc),
DigitalSignatureReference = (string?)element.Element(xc + "digitalSignatureReference"),
DigitalSignatureAlgorithm = ParseSignatureAlgorithm(element, xc),
DigitalSignatureValue = digitalSignatures.FirstOrDefault(),
DigitalSignatures = digitalSignatures,
ExpectedHash = ReadExpectedHash(element),
CompressionFlag = ParseBool(element, "compressionFlag", xc),
DefaultLocale = ReadPTLocale(element.Element(xc + "defaultLocale"), lan),
OtherLocales = element.Elements(xc + "otherLocale").Select(e => ReadPTLocales(e, lan)).ToList(),
};
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:134
- CompliancyCategory parsing uses the non-generic Enum.TryParse overload with a
typeof(...)and casts fromobject. The generic overload is simpler and avoids boxing/casting.
CompliancyCategory? comp = null;
string? compStr = (string?)element.Element(xc + "compliancyCategory");
if (compStr != null && Enum.TryParse(typeof(CompliancyCategory), compStr, ignoreCase: true, out var parsedComp))
comp = (CompliancyCategory)parsedComp;
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:200
- New parsing logic is introduced here for
IssueTime,TemporalExtent,DefaultLocale/OtherLocales, andResourceMaintenance, but current tests only assert these are null/empty. Adding a synthetic CATALOG.XML fixture covering at least one of these fields would prevent regressions (especially around date/time and locale namespace handling).
UpdateApplicationDate = ParseDate((string?)element.Element(xc + "updateApplicationDate")),
ReferenceId = (string?)element.Element(xc + "referenceID"),
IssueDate = ParseDate((string?)element.Element(xc + "issueDate")),
IssueTime = ParseTime((string?)element.Element(xc + "issueTime")),
BoundingBox = ReadBoundingBox(element.Element(xc + "boundingBox")),
TemporalExtent = ReadTemporalExtent(element, xc),
ProductSpecification = ReadProductSpecification(element.Element(xc + "productSpecification"), xc),
ProducingAgency = ReadProducingAgency(element.Element(xc + "producingAgency")),
EncodingFormat = (string?)element.Element(xc + "encodingFormat"),
DataCoverages = element
.Elements(xc + "dataCoverage")
.Select(e => ReadDataCoverage(e, xc))
.ToList(),
Comment = (string?)element.Element(xc + "comment"),
DefaultLocale = ReadPTLocale(element.Element(xc + "defaultLocale"), lan),
OtherLocales = element.Elements(xc + "otherLocale").Select(e => ReadPTLocales(e, lan)).ToList(),
MetadataDateStamp = ParseDate((string?)element.Element(xc + "metadataDateStamp")),
ReplaceData = ParseBool(element, "replaceData", xc),
NavigationPurpose = navigationPurpose,
ResourceMaintenance = ReadResourceMaintenance(element.Element(xc + "resourceMaintenance"))
};
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:745
- Comment grammar: "Permit dates" reads like a typo; should be "Permitted dates".
// Permit dates are xs:date and may carry a trailing 'Z' (e.g. 2018-03-20Z).
string trimmed = value.Trim().TrimEnd('Z', 'z');
return DateOnly.TryParse(trimmed, CultureInfo.InvariantCulture, DateTimeStyles.None, out DateOnly date)
src/EncDotNet.S100.ExchangeSets/ExchangeCatalogueReader.cs:760
- Comment grammar: "Permit times" reads like a typo; should be "Permitted times".
// Permit times are xs:time and may carry a trailing 'Z' (e.g. 13:45:30Z).
string trimmed = value.Trim().TrimEnd('Z', 'z');
return TimeOnly.TryParse(trimmed, CultureInfo.InvariantCulture, DateTimeStyles.None, out TimeOnly time)
src/EncDotNet.S100.ExchangeSets/PtLocale.cs:13
- PtLocale uses
requirednon-nullable strings, but the parser currently substitutes empty strings when values are missing. This makes it impossible for callers to distinguish "missing" vs "present but empty" and weakens the value ofrequired. Consider making these properties nullable instead and letting the reader return null when they are absent.
public sealed class PtLocale
{
public required string Language { get; init; }
public string? Country { get; init; }
public required string CharacterEncoding { get; init; }
Comment on lines
+630
to
+631
| var sigEl = wrapper.Element(S100SE + "resourceMaintenance"); | ||
| if (sigEl is null) return null; |
| MetadataDateStamp = (string?)element.Element(xc + "metadataDateStamp"), | ||
| NavigationPurpose = (string?)element.Element(xc + "navigationPurpose"), | ||
| Comment = (string?)element.Element(xc + "comment"), | ||
| DefaultLocale = ReadPTLocale(element.Element(xc + "defaultLocale"), lan), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Spec alignment
Check each spec this PR touches and confirm the relevant skill was
consulted (
.github/skills/<spec>/SKILL.md):s100-framework)s101-enc)s102-bathymetry)s104-water-level)s111-surface-currents)s124-nav-warnings)s129-ukc)Spec section references cited in code/docs:
Tests
tests/SkippableFactdotnet test --configuration Releasepasses locallyDocumentation
src/<project>/README.mddocs/if user-facing behaviourchanged
Dependencies
Directory.Packages.props(not in the.csproj)gh-advisory-databasesecurity check run for any new dependencyBreaking changes