sandstone
sandstone copied to clipboard
Add more uses to DataPoint and DataPointInstance
This pull request added:
- Storage name is now namespaced
- A
get
option toDataPointInstance
. Example:Data('storage', 'foo').select('bar').get()
->data get storage default:foo bar
-
DataCommand.get
,DataCommand.merge
,DataCommand.modify
,DataCommand.remove
are now callable, takingDataPointInstance
orDataInstance
as arguments. Example:data.modify(Data('storage', 'foo').select('bar')).set.from(Data('storage', 'foo').select('baz'))
->data modify storage default:foo bar set from storage default:foo baz
I'm not sure I see the point of those commands:
-
DataPointInstance.get
compiling to/data get ...
does not look useful to me.data get
is only useful when used withexecute.store
, in which case you would need to write the base command anyway - likeexecute.store...run.data.get.storage(...)
. And this can be avoided by using all Sandstone's abstractions, likemyScore.set(myDataPoint, scale)
. -
data.modify(Data('storage', 'foo').select('bar')).<set|remove|append|preprend|...>
can directly be reduced toDate('storage', 'foo').select('bar').<set|remove|append|preprend|...>
. Same for merge and remove. Forget
however, I can understand the usefulness when considering my previous example:execute.store...run.data.get(myDataPoint)
makes total sense.
For the namespace-thing, it's a great change, thanks.
- The
DataPointInstance.get
command is to add consistency to the system. All other operations have their own methods, onlyget
doesn't. It also makes the library a bit more object-oriented. - For the
data.modify(Data('storage', 'foo').select('bar')
thing, I totally forgot thatDataPointInstance.modify
exists, but it stills has its use: It allows a mix of minecraft command-like and object-oriented coding style. A niche use though.
- I'm afraid
DataPointInstance.get
will just confuse people, who might want to attempt things likemyScore.set(DataPointInstance.get())
, which could make sense but does not have the same purpose at all. Sandstone abstractions are not meant to be a 1-1 mirror of Minecrafts commands, but a nice abstraction layer over common use cases. Adding a seemingly useless command for the sake of consistency is not in Sandstone's interest. - Regarding this case, it's a bit more complicated. There is no use-case that isn't already covered by
DataPointInstance
's builtin methods, however it indeed offers some flexibility. Someone mixing Sandstone's abstractions with vanilla commands (which is not recommended) would benefit from this change. Since it should not confuse people (it's just an overload after all), I'll consider this change is fine and I'll merge it.
So basically, let's just remove the .get method and we're good!
I think the opposite: .get
should remain and the new function overloads may be discarded. The reason is that currently there is no object-oriented way to store a data point to another data point, this partially enables that.
Maybe adding an overload to .set
with the signature (value: DataPointInstance, storeType: StoreType, scale?: number) => void
would be better?
For .get
, we could draw a parallel with the Score
class. It does not have a get
method, yet nobody ever complained about that. Once again, the command /scoreboard players get
by itself is useless, and Score.get()
could not be combined with another command, making it useless by association. The same goes for DataPointInstance
.
Regarding .set
, there is already an overload with the signature .set(value: DataPointInstance)
. Do you think that the ability to change the storeType & the scale would be of significant value? Because I am not against such an overload, if you think this is potentially useful.
It wouldn't be of significant value, but it brings sandstone abstractions closer to full coverage