XmlWriterSettings should specify NamespaceHandling.OmitDuplicates

[lorenh]

This is minor because I have a workaround, but I believe that in GpxFile.cs line 28 you should consider adding a setting to omit duplicates when writing out GPX files using BuildString().

private static readonly XmlWriterSettings XmlWriterSettings = new XmlWriterSettings
{
    CloseOutput = false,
    Indent = true,
    NewLineOnAttributes = true,
    OmitXmlDeclaration = true,
    NamespaceHandling = NamespaceHandling.OmitDuplicates,  <=== consider adding this
};

I'm trying to match an existing GPX file with this package and currently namespaces on extensions get repeated. This is what currently gets written by the unit test code below. Notice the type element has a duplicate xmlns, even though it matches the default ns.

<gpx version="1.1" creator="airbreather" xmlns="http://www.topografix.com/GPX/1/1">
  <metadata>
    <extensions>
      <type xmlns="http://www.topografix.com/GPX/1/1">Test</type>
    </extensions>
  </metadata>
</gpx>

This is ideally what I want:

<gpx version="1.1" creator="airbreather" xmlns="http://www.topografix.com/GPX/1/1">
  <metadata>
    <extensions>
      <type>Test</type>
    </extensions>
  </metadata>
</gpx>

Here is a unit test that illustrates the issue. I can work around it for now in my code by implementing the small amount of code in BuildString() and supply my own XML Writer options, but the default writer options are private static and not exposed to be overridable. Another option would be to accept an optional XmlWriterSettings parameter to BuildString().

[Fact]
[GitHubIssue(33)]
public void ExtensionsShouldOmitDefaultNamespace()
{
    // Arrange
    XNamespace ns = "http://www.topografix.com/GPX/1/1";
    var extensions = new ImmutableXElementContainer(new[]
    {
        new XElement(ns + "type", "Test")
    });

    var metadata = new GpxMetadata(
        creator: "airbreather",
        name: null,
        description: null,
        author: null,
        copyright: null,
        links: ImmutableArray<GpxWebLink>.Empty,
        creationTimeUtc: null,
        keywords: null,
        bounds: null,
        extensions: extensions);

    var gpxFile = new GpxFile { Metadata = metadata };

    // Act
    string text = gpxFile.BuildString(null);

    // Assert
    Assert.Contains("<type>Test</type>", text);
}

[lorenh]

The more I think about it, the more I feel that adding the ability to optionally pass an XmlWriterSettings to BuildString() is the better option here. There are potentially too many XML tweaks people will want to make to force them to a hard-coded default.

public string BuildString(GpxWriterSettings gpxSettings, XmlWriterSettings xmlSettings = null) { ... }

Doesn't matter much to me what direction you go, I need to use WriteTo() because I'm writing to a memory stream, so BuildString() is dead to me :-)

[airbreather]

potentially too many XML tweaks people will want to make to force them to a hard-coded default.

The string methods (both parsing and building) are intended to be that way: the intent is that all users who need something else will need to provide their own XmlReader / XmlWriter instance.

That said, I agree that NamespaceHandling.OmitDuplicates makes more sense as a default, so I intend to set that flag in the hardcoded default and leave it at that.

[lorenh]

Agreed wholeheartedly with your approach here. Thanks for addressing it so quickly!