Skript icon indicating copy to clipboard operation
Skript copied to clipboard

Shortened Timespans (again)

Open Asleeepp opened this issue 1 year ago • 1 comments

Description

This PR aims to add an option to provide a shortened version of a Timespan. Unlike my previous PR of this exact thing, this uses the lang file, and does not require the Command context, as I felt making this exclusive to commands was a bit silly. Not sure if this will require a test, or how I could even make a test for this. If it does, please tell me, (and some guidance would be helpful). and I will make one.


Target Minecraft Versions: any Requirements: none Related Issues: none

Asleeepp avatar Aug 28 '24 00:08 Asleeepp

Oh, lovely, the tests are failing, guess i screwed something up

Asleeepp avatar Aug 28 '24 00:08 Asleeepp

The code looks ok, but I'm a bit concerned about allowing this outside commands because it feels like it could be weird and ambiguous in code. My concern is mainly that m is the international standard abbreviation for metres (a unit of measurement for any americans out there), I can guarantee this will trip up some beginners doing things like the block 20m in front of player and wondering why it's not working. I think it's also worth noting that Minecraft uses d and s as number type tokens in its stringified NBT format (and in fact some real languages use D as a double token) so I can see somebody getting confused by 0.5d or whatever, but I think that's a much lesser concern.

If the others are happy with this then I'm fine with it, but if it was me I would probably just restrict this to commands or text -> timespan parsing where a shorthand (e.g. 1h30m, 3d2y) feels more appropriate.

The original PR was locked behind command context, but i removed that requirement with this new PR, as I personally felt that locking this behind just commands felt a little bit silly, but you do have some valid arguments, if anyone else agrees with Kenzie on this, i'll lock it behind commands only

Asleeepp avatar Sep 09 '24 16:09 Asleeepp

if anyone else agrees with Kenzie on this, i'll lock it behind commands only

I'm not like totally against it, I just feel like it's the sort of thing that might bite us down the line, would be interested to see others' opinions :smile:

Moderocky avatar Sep 09 '24 16:09 Moderocky

if anyone else agrees with Kenzie on this, i'll lock it behind commands only

I'm not like totally against it, I just feel like it's the sort of thing that might bite us down the line, would be interested to see others' opinions 😄

Yes i think it'd be better as command context only. I understand the need for brevity in commands (and it's less likely to cause issues there), but in actual code i think writing out the units makes much more sense, makes code more readable, prevents possible future conflicts, etc.

sovdeeth avatar Sep 15 '24 00:09 sovdeeth