-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shapefile IO refactoring #23
Comments
Can you explain what you mean further, please? The NetTopologySuite.IO package only depends on NetTopologySuite (and thus GeoAPI). Or are you suggesting that there should be some kind of additional package NetTopologySuite.IO.ShapeFile or NetTopologySuite.IO.GeoTools or something, which only depends on GeoAPI? If so, I like this idea, if you can rip out its uses of other stuff from NetTopologySuite proper. From memory, I think the only major thing the NetTopologySuite.IO.GeoTools uses from NetTopologySuite proper is the stuff from the NetTopologySuite.Features namespace, so it sounds possible. Doing it in a backwards-compatibility would be a nuisance.
I agree that shapefile reading / writing support in .NET Standard is important enough to justify. Unless you get really aggressive with it, I think the lowest you can do is .NET Standard 2.0 (especially for the use case that I think you have in mind, which requires all the APIs that make me say that).
Looking at NetTopologySuite.IO.GeoTools, I don't really think there's all that much of this... am I looking in the wrong place? |
There is IO.GeoTools but it has a dependency on IO.ShapeFile. I think we need to define what does what here too because it's a bit confusing having this:
Not suggesting only depending on GeoAPI, I don't think that's a good choice because we need the geometries in NTS. This code I don't think is IO and dumps a shapefile into a database: Not sure where this would go, does it even belong in NTS? I think we should have one package, on top of NTS, that supports IO for shapefiles:
I suggest removing the functionality from GeoTools and adding it to ShapeFile. |
Got it... makes sense.
I see now. Agreed, the SQL stuff in that file looks like crap.
This was what I meant when I said "really aggressive", particularly the "and attributes" part. ShapefileDataReader was designed to surface the results using the interface that already exists in System.Data.IDataReader. This design dates back to 2002-2003 in Geotools.NET (which, not too coincidentally, was written by some other employees of the company I currently work for). I don't think that's the best design possible, but it has its own advantages like being able to use it with APIs that consume arbitrary IDataReader instances. Notably, I think it's a big deal that the *.dbf format is very much a good match with IDataReader. Anyway, redoing that means a lot of rework and finding an alternative design that works, especially if you want to maintain backwards-compatibility since you'd probably have to at least consume that other design in the shape of IDataReader for unusual kinds of consumers.
I'd personally put it into the NTS.IO package, not a big difference.
It's already used in the NetTopologySuite.IO project / DLL, in ShapefileStreamProviderRegistry.
Agreed... actually, @FObermaier's |
I'll start on a pullrequest somewhere next week. I agree that we should include all we can in NetTopologySuite.IO but only when there aren't any extra dependencies required, for example Newtonsoft.Json for the GeoJson IO. should stay seperate. |
I'd like to emphasise that shapefile support is THE feature I want from NTS, now that we have .NET Core compatibility, so I am looking forward for this ticket to get resolved . Keep up the good work! |
Thumbs up to all of this :) Also, would it be possible to extract an interface for the |
It has taken a bit longer than I expected but I created a small separate project here: https://github.com/itinero/nettopologysuite.io.portableshape This supports netstandard1.3 and has no dependencies other than the main NTS package so it can easily be made into a pull-request for the main NTS repo here. But it's a pretty big change, it would mean removing IO.GeoTools and converting IO.ShapeFile. Let me know what you all think so I can start working on a pull request. In the meantime feel free to test the package containing all this: https://www.nuget.org/packages/NetTopologySuite.IO.PortableShape/ (will be removed again after merging here) |
I've briefly looked over this... it looked like the main thing you did was take out the If so, I'm not totally sure about this kind of a change. The legacy design is really awkward, and it kinda has to be in order to fit the shape of Perhaps a better course of action might be to just look into making the existing design work with the In theory, it would be possible to make a lower-level API for shapefiles, and then the current |
I'm in favour of a pragmatic approach and take what we have now, simplify it and add some extra functionality making it easier to work with. Yes, I basically took out System.Data and added a Feature, and Attributes property to the reader to get a feature per row without having to write your own code. System.Data doesn't really add value. I also concentrated all the code needed in one project but I had to take code from 3 different projects in the original NTS code, most of the code is used exclusively for shapefile IO and should be removed. So, basically I cleaned up a lot of the code just by ripping out the shapefile IO functionality. We could also make the reader implement We can now have one method that takes a filename and returns an The risk of a full redesign is that it will never happen. I'm not willing to put in the effort. Taking in this code, improving it a little bit, releasing it, doesn't stop a redesign in the future. We could also leave the IO code in seperate repos and let it have it's own release cycle. As for issue #154, we can work on that now because the code has been cleaned up. I think the advantages are clear:
Basically I think we should remove IO.GeoTools and replace IO.ShapeFile with this new project or keep it a seperate project but then under the NTS organization and remove IO.GeoTools and IO.ShapeFile here. |
I'm not sure I agree... I can easily imagine someone on .NET 4.x using
The main thing I'm concerned about with this is that consumers that use the existing shapefile API would almost certainly have to adapt to this change, for questionable benefit. IMO, if we're going to (potentially) break downstream consumers, then it should be for a good reason, especially if we're not offering them anything new, especially-especially if the API is still going to be a clunky mess.
Agreed, that's the risk. As things stand right now, do you see any reasons why it wouldn't work to just add |
System.Data was just used to implement one interface, IDataReader. So still not convinced, if we take a dependency it should, as far as I'm concerned be related to the core goal of the package, and that is reading and writing shapefiles. As for the API, nothing has changed that's related to the core funtionality of what this should do, read/write shapefiles. The only potentially breaking change is that netstandard doesn't have a default encoding because it doesn't define a platform. That will remain the case for netstandard2.0. Enabling bulk copy could be a sample project we publish, it's perfectly doable without having a system.data dependency. Otherwise that would imply we would need to implement IDataReader in all IO packages. |
FWIW, 4b18538 makes Not that this method couldn't be made to work (it wouldn't even be that hard), just I didn't want to add the package reference to System.Data.SqlClient that it would take to make it work. |
Can this be closed, or should I move it to the NetTopologySuite.IO.ShapeFile project? |
Looks like this can be closed! Excellent work being done these days in NTS! 💯 |
I want to change the way the shapefile read/write works now:
Anyone have any feedback? Should I do this outside of the main NTS project, create a project/package in a new repo?
The text was updated successfully, but these errors were encountered: