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

IAttributesTable could support read-only instances better #16

Open
airbreather opened this issue Feb 9, 2023 · 0 comments
Open

IAttributesTable could support read-only instances better #16

airbreather opened this issue Feb 9, 2023 · 0 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@airbreather
Copy link
Member

airbreather commented Feb 9, 2023

NetTopologySuite/NetTopologySuite.IO.GeoJSON#117 was a bit of an awkward spot.

I would have preferred to have pointed to an IsReadOnly property and recommended copying the read-only IAttributesTable into a new editable instance, since I don't expect there to be many actual use cases where you need to edit the table in-place. After all, the table itself is little more than a way for us to expose the data that's present in the source file, and any edits that you make to the table instance do not directly get written back to that source file.

However:

  • No IsReadOnly property (or related thing) exists, and so a negative response would have been unfair: it's unreasonable that "you just have to know" that the 4STJ implementation of IAttributesTable is read-only, especially when its Newtonsoft.Json counterpart did not work that way.
  • There is no simple, built-in way to take an existing IAttributesTable instance and use it to initialize a new IAttributesTable object that's writable and has the same initial values, so even if had allowed myself to say "you just have to know", it would have still added an unreasonable amount of friction to the upgrade process, because a downstream consumer should not be expected to implement something like that on their own.

Things Not To Do... Yet...?

These would all be breaking changes:

  1. add bool IsReadOnly { get; } directly to the IAttributesTable interface
  2. add IAttributesTable ToWritable(); directly to the IAttributesTable interface
  3. make an IReadOnlyAttributesTable interface, and change IAttributesTable to extend it

Suggestions

  1. Create an IReadOnlyAttributesTable interface. Implement it in this project's AttributesTable class. When the new version of this library is published, implement that interface in the classes that implement this interface for all our other IO libraries.
  2. Create a public static AttributesTable ToWritable(this IAttributesTable @this) extension method that creates a new empty AttributesTable and initializes it using values copied from @this.
    • Alongside this extension method, create a new interface, something like ICanCopyToNewWritableAttributesTable (better name???). It only has one method: void CopyTo(AttributesTable table);. The extension method would check if @this implements that interface. If it does, then we call that method. Otherwise, we do the copying ourselves. As above, implement this interface in all of our own implementations of IAttributesTable, for the best user experience.

I'm intentionally not jumping on this right away, because I think there's some room for some better ideas, and/or it might be a good first issue for someone new. (edit to add:) ...also, I still hold on hope that something might eventually happen with #6, which would affect this... even though, realistically, it's probably too big a change for too small a benefit...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant