Updates for v2.

[airbreather]

• All CoordinateSystems types are gone (resolves Remove CRS stuff #7)
• The extension methods dealing with ID values on IFeature instances are gone
  • to be moved to GeoJSON
• (I)AttributesTable now has GetOptionalValue (resolves Simplify fetching values for optional attributes in (I)AttributesTable #3)
• FeatureCollection now inherits from Collection
• [Serializable] is now implemented via ISerializable

[airbreather]

I'm not positive about what to do to handle what used to be FeatureCollection.Type.

It's really specific to GeoJSON, so my current plan is to have GeoJSON use something like GeoJsonFeatureCollection with:

• FeatureCollection Features { get; } and
• GeoJsonObjectType Type { get; }

Maybe it does make sense for it to go here instead, but it'll be easy enough to add later.

[FObermaier]

I dislike the removal of IFeature and IAttributesTable interface.
I have lots of special purpose feature classes in my code that I can't use with this PR as is.

[FObermaier]

I dislike the removal of IFeature and IAttributesTable interfaces.
In my code I have lots of special purpose feature classes that I can't use anymore.
As this is not just a supporting library for NetTopologySuite.IO projects I prefer keeping them or at least provide a base class for feature.

[airbreather]

In my code I have lots of special purpose feature classes that I can't use anymore.
As this is not just a supporting library for NetTopologySuite.IO projects I prefer keeping them or at least provide a base class for feature.

OK. Assuming the idea is similar to NetTopologySuite.IO.GeoJson.Test.Usage/Road.cs in that other project, there are advantages and disadvantages to that approach.

I personally favor the way NetTopologySuite.IO.GPX handles (I)Feature, but of course "I personally favor" isn't a good enough reason to knowingly force people to completely rewrite working software components. I'll come up with a lighter treatment of (I)Feature / (I)AttributesTable.

[airbreather]

IFeature is back.

Do we need to support modifying the feature through the interface? If not, then IReadOnlyDictionary<string, object> may be a good compromise for #6: it's much easier to implement than IDictionary<string, object>, it's still a standardized interface, and implementations of the interface can still be mutable.

[airbreather]

@FObermaier ping ^

Once NetTopologySuite.Features is settled, I can get to the other IO libraries, but I don't want to merge this one until I know you're OK with it.

[mhosman]

Any news on this one? Thanks!

[airbreather]

OK, I haven't heard back in more than a couple of weeks, and I really want this to coincide with .NET Core 3.0, so I'm going to make a few decisions:

• IFeature is going to be read/write like it was before
• IAttributesTable is still going away, but as IDictionary<string, object> instead.
• To help transition any existing subclasses of IAttributesTable, I'm also going to add a base class that fully implements IDictionary<TKey, TValue> with only a few abstract methods.

[FObermaier]

Sorry for the delay, I have been on vacation.
I welcome IFeature being back. I'm still not happy with IDictionary<string, object> as a replacement for IAttributesTable.

[airbreather]

I'm still not happy with IDictionary<string, object> as a replacement for IAttributesTable.

Can you please elaborate on what the issue is, now that there's a DictionaryBase<TKey, TValue> base class that only requires implementing a handful of members?

[airbreather]

I'm still not happy with IDictionary<string, object> as a replacement for IAttributesTable.

Also, what about the idea of the IFeature interface being read-only, including Attributes being IReadOnlyDictionary<string, object>, but then allowing implementations to perhaps be read/write?

IReadOnlyDictionary<string, object> is much easier to implement, and whoever creates the instances of the concrete implementation of IFeature can edit it before sending it off to whatever API asks for an instance of IFeature.

[FObermaier]

@airbreather neither IReadonlyDictionary<string,object> nor IDictionary<string,object> have a GetType(string key) function on it to query required types for potentially unset/null members. For the handling my IFeature implementing classes I found this to be extremly usable. Furthermore I have no need for any IEnumerable on my attributes.

I like the simplicity of IAttributesTable and don't see a reason to remove it, other than to use a core interface.

How about:

public interface IAttributesTable : IReadOnlyDictionary<string, object>
{
    new object this [string key] { get; set; }
    Type GetType(string name);
}

[airbreather]

neither IReadonlyDictionary<string,object> nor IDictionary<string,object> have a GetType(string key) function on it to query required types for potentially unset/null members. For the handling my IFeature implementing classes I found this to be extremly usable.

Hmm, the concept of "unset/null" actually suggests that neither IDictionary<string, object> nor IAttributesTable is a good match for all kinds of attribute tables. Perhaps we actually have to consider two different kinds of attribute tables:

1. Tables that can contain any number of arbitrary attributes per feature (e.g., GeoJSON)
2. Tables whose valid values are restricted by something external to the individual feature (e.g., shapefiles, GPX, relational databases)

IReadOnlyDictionary<string, object> seems to cover both kinds precisely, but it doesn't support editing. The problem with IDictionary<string, object> and IAttributesTable is that there's no way to tell which of the situations you're in without just trying it and hoping it works.

Do we actually need to support editing an instance of IFeature using just the interface? My instinct is that whoever is creating instances of IFeature has enough information to be able to edit it via the concrete type instead of needing to go through this interface.

If we do need to support editing via the interface (and I kinda hope we don't...), perhaps we should have more than one interface, something like these (names are tentative):

• IReadOnlyFeature has Geometry, Envelope, and IReadOnlyDictionary<string, object>
• IEditableFeature extends IReadOnlyFeature and supports editing the Geometry and the Envelope.
• IEditableFeatureWithFixedAttributes extends IEditableFeature and supports editing a fixed list of attributes, where each attribute has at least a name and CLR Type.
• IEditableFeatureWithDynamicAttributes extends IEditableFeature and essentially replaces the IReadOnlyDictionary<string, object> with an IDictionary<string, object>.

[FObermaier]

Why? I did get along with IAttributesTable. If it is such a big deal to make a feature's attributes adhere to some System.Collections dictionary interface than go ahead, and I'll make and use my own IFeature interface using IAttributesTable.

This interface has been around for quite some time and I'm sure there are implementations around (besides mine) that actively use it. We force everyone of these to make (substantial) adaptations to use NTS v2. I don't recall any NTS user complain about the IFeature interface and IAttributesTable, so why?.

[airbreather]

I'm going to go ahead and bring back IAttributesTable, for the sake of getting NTS v2 out the door.

I'd still like to have a type hierarchy that's a better match for the problem(s) it's trying to solve, but I don't think the existing solution is bad enough to justify the costs of any of the alternatives I've considered over the course of this discussion.

Responses below:

Why?

At first, I opened #6 to ask "why not?". It wasn't obvious what IAttributesTable was doing for us that wasn't already handled by IDictionary<string, object>, and the use of Hashtable in AttributesTable suggested that it may have been written as a way to get string into method signatures, at a time before .NET got generics.

With v2 giving a rare opportunity to make intentional breaking changes, I didn't want to miss the opportunity to eliminate this, if it was indeed redundant with what the system already offers us.

Through our discussions, you persuaded me that this interface is not fully redundant (sorry if this fact wasn't clear from my previous comment, I did rush through it), but after thinking through the things it offers that IDictionary<string, object> does not, I started to see two different kinds of "attributes tables". Readers could consume both kinds of tables the same way, but writers don't have any way of knowing whether or not they're looking at:

• the kind of table that knows the type of attributes before their values have been set, or
• the kind of table that acts more like a wrapper around Dictionary<string, object>

So I posited that a better way to model this solution would encode that information in the type system... and noted that it might be good enough to just have the interface(s) support reading.

That's what led up to my previous comment on this thread.

If it is such a big deal to make a feature's attributes adhere to some System.Collections dictionary interface than go ahead, and I'll make and use my own IFeature interface using IAttributesTable.

It's not just about that, it's about finding appropriate solutions to the problems that we set out to solve. A solution that solves the problem is infinitely better than one that doesn't. IDictionary<string, object> probably doesn't. If IFeature needs to support writing, then IReadOnlyDictionary<string, object> doesn't either. Everything else is secondary to that.

I don't recall any NTS user complain about the IFeature interface and IAttributesTable, so why?.

#3 and #6 are complaints that resulted from me trying to work with these. #6 specifically is one I had a few years back when switching our application at work to use NTS, and never bothered to raise up because that part of the code doesn't really get used much anyway.

[FObermaier]

Thanks @airbreather for all your work on this and for pushing NTS to v2.
Perhaps we can bring back (and extend) AttributesTableTest.

[DGuidi]

thanks @airbreather for this 👍