Rollczi / LiteCommands

☄️ LiteCommands - Command framework for Velocity, Bukkit, Paper, BungeeCord, Minestom, Sponge, Fabric, JDA and future implementations.
https://docs.rollczi.dev/
Apache License 2.0
147 stars 22 forks source link

GH-233 Add Instant argument support #252

Closed BlackBaroness closed 1 year ago

BlackBaroness commented 1 year ago

Closes #233

BlackBaroness commented 1 year ago

Build and tests are ok locally

Rollczi commented 1 year ago

You should

  1. See implementation from 2.x version (DateTimeFormatter is used there) https://github.com/Rollczi/LiteCommands/blob/v2.x.x/litecommands-core/src/main/java/dev/rollczi/litecommands/argument/basictype/time/InstantArgument.java

  2. example of multiple argument with 3 parts https://github.com/Rollczi/LiteCommands/blob/master/litecommands-bukkit/src/dev/rollczi/litecommands/bukkit/argument/LocationArgument.java

BlackBaroness commented 1 year ago

Maybe we should add timezone support? Most users won't understand that they need to enter UTC time

Rollczi commented 1 year ago

Maybe we should add timezone support? Most users won't understand that they need to enter UTC time

Instant is used to define moment in UTC (always GMT+0)

You can use OffsetDateTime to define offset (e.g. GMT+2, GMT+4) or ZonedDataTime to define timezone (e.g. Europe/Warsaw) LocalDateTime is also a good solution

image

BlackBaroness commented 1 year ago

You are right. I was asking because ISO_8601 supports timezones, so I thought it's smart to support them too. But there are even no methods to do that, because it doesn't make sense

I will complete this PR shortly. The only problem is tests are failing without reason (but passing locally), but we will look at it after DateTimeFormatter impl

Rollczi commented 1 year ago

The only problem is tests are failing without reason (but passing locally)

this may be caused by lack of control over the time zone as it is done in DateTimeFormatter

BlackBaroness commented 1 year ago

@Rollczi I can't understand why this doesn't work. Firstly, String.join(" ", rawInput.next(2)) throws ConcurrentModificationException. Secondly, command always fails, even if I put if (true) return ParseResult.success(Instant.now())

Rollczi commented 1 year ago

@Rollczi I can't understand why this doesn't work. Firstly, String.join(" ", rawInput.next(2)) throws ConcurrentModificationException. Secondly, command always fails, even if I put if (true) return ParseResult.success(Instant.now())

rawInput.next(2) is mutable view of original list. I missed that part

BlackBaroness commented 1 year ago

Can be merged after successful build. Thanks, @CitralFlo and @Rollczi !