search

NetTopologySuite / NetTopologySuite.IO.Esri

BSD 3-Clause "New" or "Revised" License
35 stars 18 forks source link

Add tests from NetTopologySuite.IO.ShapeFile repository #5

Closed KubaSzostak closed 1 year ago

KubaSzostak commented 2 years ago

Tests which are no longer relevant (for example test of decommissioned GDBReader) are marked with following comment:

// TODO: Remove no longer relevant test
KubaSzostak commented 2 years ago

Questions related to Issue27Fixture.cs:

  1. Is IShapefileFeature + ShapefileFeature required? Currently they are not implemented.
  2. Currently ShapefileReader implements IEnumerable<Feature>. Should it be changed into IEnumerable<IFeature>? Or maybe IEnumerable<IShapefileFeature>?
airbreather commented 2 years ago

Currently ShapefileReader implements IEnumerable<Feature>. Should it be changed into IEnumerable<IFeature>? Or maybe IEnumerable<IShapefileFeature>?

The T parameter in IEnumerable<out T> is covariant, so if it implements IEnumerable<Feature>, then it can automatically work for any caller who wants IEnumerable<Feature> or IEnumerable<IFeature>, or any other interface that Feature implements.

The main reason why you might not want to implement IEnumerable<Feature> is if you want to be able to change the concrete feature type later without breaking callers, in which case go ahead and go with IEnumerable<IFeature> (or some sub-interface that you might want to use instead). As long as the library hasn't shipped yet (it hasn't, right?) it's not a big deal.

KubaSzostak commented 2 years ago

Hi guys, I've started porting tests from NetTopologySuite.IO.ShapeFile into this repository. I'm porting them one-by-one. As new code is just different new tests must be updated to reflect the new code. Not always it's possible to port the test and have exactly the same logic as before. Therefore it would be great if someone could take a look on it and check if I'm going in right direction.

KubaSzostak commented 2 years ago

Thanks for update @airbreather. The library isn't shipped yet. I can easily change IEnumerable<Feature> into IEnumerable<IFeature>. If having the property FeatureId is not a requirement, I would like to avoid adding IShapefileFeature and related class. Is that OK? 

// Code from NetTopologySuite.IO.ShapeFile
public interface IShapefileFeature : IFeature
{
  long FeatureId { get; }
}
airbreather commented 2 years ago

To be honest, I'm only paying very little attention to this (and most other NTS stuff) these days, since my free time has been stretched many other different ways for the past ~year or two.

If having the property FeatureId is not a requirement, I would like to avoid adding IShapefileFeature and related class. Is that OK?

re: IShapefileFeature and related things, the only productive thing I have to say is that I dislike much of the API in the legacy NetTopologySuite.IO.ShapeFile package. It's clunky, unintuitive, and at times just... weird, in ways that the already weird shapefile spec doesn't force it to be. So from my perspective, I consider NetTopologySuite.IO.Esri to be a completely new thing without any particular constraints that it needs to match the legacy API. In case that helps answer