Skip to content
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

Support a variant of JsonElementAttributesTable that can be modified in-place, at a cost to reading performance. #118

Merged
merged 13 commits into from
Feb 9, 2023

Conversation

airbreather
Copy link
Member

@airbreather airbreather commented Feb 1, 2023

Resolves #117

JsonElementAttributesTable is just fine as it is, so it hasn't been touched. Instead, I've created a new table type that's backed by the (relatively) new (to us) JsonObject, from the System.Text.Json.Nodes namespace.

This design is optimized for people who want to take an IFeature that comes from 4STJ and make relatively few changes to its Attributes before handing it off somewhere else.

I would have personally preferred if IFeature / IAttributesTable had been read-only, as it seems unlikely that someone actually needs to be able to modify the objects created by our I/O helpers. However, there is at least one consumer who has come to rely on being able to do this in the Newtonsoft.Json version, and so it seems reasonable to provide a more straightforward upgrade path.

Especially since, unlike System.Collections.Generic.ICollection<T>, we don't document that instances might sometimes be read-only, nor do we give anything like IsReadOnly that someone can check to discover whether or not a particular instance can be modified.

  • System.Collections.Generic.ICollection<T> is almost as bad, to be fair: it does have this documentation and IsReadOnly that you can check, but based on my experience, I would wager that a typical developer is likely to just assume that every instance of ICollection<T> is writable (if they even know about IsReadOnly at all!) and write code that's problematic in exactly the same way... but at least there, I could argue that it's their fault for not reading the warning labels...

@airbreather airbreather changed the title Support editing a JsonElementAttributesTable in-place. Support a variant of JsonElementAttributesTable that can be modified in-place, at a slight cost to reading performance. Feb 5, 2023
@airbreather airbreather changed the title Support a variant of JsonElementAttributesTable that can be modified in-place, at a slight cost to reading performance. Support a variant of JsonElementAttributesTable that can be modified in-place, at a cost to reading performance. Feb 5, 2023
@airbreather airbreather marked this pull request as ready for review February 5, 2023 15:43
@airbreather
Copy link
Member Author

@HarelM This is what I've got for #117. Once it's merged, a prerelease package will automatically be published to https://www.myget.org/feed/nettopologysuite/package/nuget/NetTopologySuite.IO.GeoJSON4STJ.

If you wish to provide feedback before then, now is your chance.

@HarelM
Copy link
Contributor

HarelM commented Feb 6, 2023

This looks great! Thanks!
I would reccomend adding some information in the readme of this repo or the wiki (or both) to allow developers to know that this option exists, how to use it, maybe some migration notes from Newtonsoft to STJ etc.
Keep up the good work!

Copy link
Member

@FObermaier FObermaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @airbreather!

@airbreather airbreather merged commit 28502d5 into develop Feb 9, 2023
@airbreather airbreather deleted the gh-117 branch February 9, 2023 10:18
@HarelM
Copy link
Contributor

HarelM commented Mar 20, 2023

Any chance for an official release with this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't modify STJ's Attribute Table
3 participants