sandstone icon indicating copy to clipboard operation
sandstone copied to clipboard

Add more uses to DataPoint and DataPointInstance

Open ncfumction opened this issue 3 years ago • 6 comments

This pull request added:

  • Storage name is now namespaced
  • A get option to DataPointInstance. Example: Data('storage', 'foo').select('bar').get() -> data get storage default:foo bar
  • DataCommand.get, DataCommand.merge, DataCommand.modify, DataCommand.remove are now callable, taking DataPointInstance or DataInstance 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

ncfumction avatar Jun 16 '21 04:06 ncfumction

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 with execute.store, in which case you would need to write the base command anyway - like execute.store...run.data.get.storage(...). And this can be avoided by using all Sandstone's abstractions, like myScore.set(myDataPoint, scale).
  • data.modify(Data('storage', 'foo').select('bar')).<set|remove|append|preprend|...> can directly be reduced to Date('storage', 'foo').select('bar').<set|remove|append|preprend|...>. Same for merge and remove. For get 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.

TheMrZZ avatar Jun 16 '21 08:06 TheMrZZ

  • The DataPointInstance.get command is to add consistency to the system. All other operations have their own methods, only get 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 that DataPointInstance.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.

ncfumction avatar Jun 16 '21 08:06 ncfumction

  • I'm afraid DataPointInstance.get will just confuse people, who might want to attempt things like myScore.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!

TheMrZZ avatar Jun 16 '21 09:06 TheMrZZ

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?

ncfumction avatar Jun 16 '21 13:06 ncfumction

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.

TheMrZZ avatar Jun 17 '21 10:06 TheMrZZ

It wouldn't be of significant value, but it brings sandstone abstractions closer to full coverage

ncfumction avatar Jun 17 '21 11:06 ncfumction