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

immutable objects #81

Open
bergos opened this issue Aug 12, 2016 · 15 comments
Open

immutable objects #81

bergos opened this issue Aug 12, 2016 · 15 comments

Comments

@bergos
Copy link
Member

bergos commented Aug 12, 2016

Are objects created by the DataFactory immutable? How should we write that in the spec? I think we said we don't define the properties read-only to avoid forcing the usage of Object.defineProperty in the implementations.

@blake-regalia
Copy link
Contributor

I agree that Terms should generally be immutable, but I'm not totally convinced this is a justified limitation to impose in the specification. For example, pretend that an implementation wants to be extremely simple to use - so they define a setter method on Literal Terms to allow changing its value like so: someLiteral.value = 'new value';. Then perhaps they automatically perform SPARQL updates on behalf of the change.

@timbl
Copy link

timbl commented Aug 15, 2016

I oppose being able to change the value. it should be an error or an error not caught
For example a store may have index entries which become orphaned, and future queries for the new value will fail. This applies imho to simple types literals and symbols and bNodes. Complex types like Collections and Graphs and datasets may have to be more complicated

@bergos
Copy link
Member Author

bergos commented Aug 15, 2016

@timbl Yes, the indexing stuff is one reason. Another reason is the behavior when a Term instance is used multiple times in one or more Quads/Datasets. Should a value change update all Quads/Datasets?

@blake-regalia Interesting use case. If we define the objects should be handled like immutable objects without throwing an error, that implementation would still pass the tests.

I think we have three options. I will add a comment for each, please use the +1 feature to vote for it.

@bergos
Copy link
Member Author

bergos commented Aug 15, 2016

  • don't define objects immutable

@bergos
Copy link
Member Author

bergos commented Aug 15, 2016

  • define objects immutable and calling the setter method should throw an error

@bergos
Copy link
Member Author

bergos commented Aug 15, 2016

  • define objects immutable without throwing an error

@dmitrizagidulin
Copy link
Contributor

I agree with the reasoning that objects should be immutable. I do think that if we add that to the spec, though, we also need to define (or at least discuss, in the specs) a way to update term values in an immutable fashion. (Meaning, the usual functional programming style of doing something like Term .fromTerm( oldTerm ) or Term .newTerm( oldTerm ), which would return a copy of the old term, with the value changed, but with everything else remaining the same (like the language and datatype of a literal), and with indexes updated accordingly).

@RubenVerborgh
Copy link
Member

Immutability seems the way to go. However, I think implementations should be allowed to throw an error when the user tries to manipulate. It could be confusing if users try to do triple.subject = otherSubject but nothing happens. But it does not have to be made mandatory for me (because indeed, that would force accessors).

With regard to @dmitrizagidulin's proposal, I would propose having this in the constructor rather than as a separate method, or perhaps leaving this entirely up to the library. One can always default to .triple(other.subject, other.predicate, myObject).

@bergos
Copy link
Member Author

bergos commented Oct 13, 2016

We agreed in the call that Quads and Terms should be immutable but we don't define the behavior for write access. The High Level API may define more details.

@dan-f
Copy link

dan-f commented Oct 27, 2016

I'm not sure if this issue is only about Quads and Terms, but I'd also like to mention that I feel strongly that the Dataset, whether or not it is standardized, should be immutable.

@k00ni
Copy link
Contributor

k00ni commented Nov 24, 2018

What is the agreement here?

@bergos wrote:

Are objects created by the DataFactory immutable?

But the prefered poll option is:

define objects immutable without throwing an error
(with 4 votes currently)

And in the end

We agreed in the call that Quads and Terms should be immutable but we don't define the behavior for write access.

Please clarify: do you mean (1) that each instance of Term or one of its subclasses is immutable? Or (2) just the ones created by the DataFactory?

Point (1) implies that instances from the following are immutable too: Quad, Triple, NamedNode, Literal, BlankNode, Variable and DefaultGraph.

Point (2) implies that instances have to behave different, if created by DataFactory, dont they? Meaning, that they may throw an exception if the values gets changed anyway.

(ref #130)

@bergos
Copy link
Member Author

bergos commented Nov 27, 2018

Using the datafactory to create Term + subclasses and Quad is the only official way. And all of them are immutable.

Factory functions (e.g., quad()) or methods (e.g., store.createQuad()) create instances.

That's the sentence for "only way" in the spec, but I think we could improve it a little bit.

@vhf
Copy link
Member

vhf commented Nov 28, 2018

However, I think implementations should be allowed to throw an error when the user tries to manipulate. It could be confusing if users try to do triple.subject = otherSubject but nothing happens.

This feels like an important point. In my opinion using a lib implementing a spec should save oneself the tough task of learning all details of the spec.

It can have far reaching consequences, for instance using a lib to work with rdfjs datasets I can see myself doing something like:

someDataset.forEach((quad) => {
  if (quad.subject.equals(foo)) {
    quad.subject = bar
  }
})

without knowing it violates the spec because there would be no error/warning, and later on seeing something unexpectedly break making it very hard to trace the bug back to the spec.

@iddan
Copy link

iddan commented Jan 25, 2019

Any term object can go through Object.freeze(), Object.seal(), and all its properties can be nonwritable.

@awwright
Copy link
Member

I've got a lot of code with the assumption that nodes are immutable. This would be a good guarantee.

What if this has a negative performance impact? (IMO I don't think we should be worrying about performance impact, as this is subject to frequent change, premature optimization being the root of evil, etc, etc)

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

No branches or pull requests