Implementation of a Command printing the release time of a build #47

DManstrator · 2019-06-26T23:20:40Z

Description

This PR adds a command telling when a JDA build was released.

If no version was provided, the latest successful build will be used instead.

Potential Errors like parsing the User Input and IOException of JenkinsApi#getBuild are correctly handled.

Used Code Style was respected.

Example Preview

kantenkugel

I think this is a good idea but why not just add that to the changelog command and add some aliases like !build (if it doesn't exist already). That way you have everything in one place.

...src/main/java/com/almightyalpaca/discord/jdabutler/commands/commands/DateVersionCommand.java

DManstrator · 2019-06-27T19:25:10Z

Didn't see your comment before, sry.

I think this is a good idea but why not just add that to the changelog command and add some aliases like !build (if it doesn't exist already). That way you have everything in one place.

Don't know if you saw it but Alpaca kind of requested that, I just executed it (Link to Discord Chat). :D

Actually I see this more like of a fun command where the changelog command is a serious one. If you really want I can do it like that but I don't think it's a good idea, the changelog already has a lot of contents and too many information aren't good imo.

Added Logging for Exception occurrences, simplified getting a formatted time based of a OffsetDateTime and removed command name from List of Aliases

DManstrator · 2019-06-30T10:30:29Z

I wasn't home over the weekend and didn't want to commit without your response so I did it now, sorry for any inconveniences.

kantenkugel

Just a minor issue with using SLF4J incorrectly

...src/main/java/com/almightyalpaca/discord/jdabutler/commands/commands/DateVersionCommand.java

kantenkugel · 2019-06-30T14:35:10Z

I'm just gonna let someone else (maybe Mighty) look over it before merging

DManstrator · 2019-06-30T14:55:08Z

Yeah, that's fine. :)

Almighty-Alpaca

I suggested the command so you can show people how ancient their JDA versions are more easily. That said I also like @kantenkugel's idea to add it to the changelog command, although the focus there isn't on the date anymore. It can always be added to that command additionally.

Almighty-Alpaca · 2019-07-01T15:25:59Z

...src/main/java/com/almightyalpaca/discord/jdabutler/commands/commands/DateVersionCommand.java

+
+public class DateVersionCommand extends Command
+{
+    private static final String DATE_FORMAT = "yyyy-MM-dd; HH:mm:ss";


Would be nice to include the timezone here as the time is kinda useless without it

That reminds me... I think i added the DurationUtils i wrote to Butler... Which means we could print the difference version-release -> now in some 1w, 3d, ... format as well (next to datetime).
Possibly needs to be expanded with month/year handling cuz i don't think i did those

@Almighty-Alpaca Which Timezone should I use then? Or do you mean I should just add O to the Pattern?
@kantenkugel Can you explain the includeIntermed param of the formatDuration function? I saw no difference when trying out both.

Currently it looks like this:

But this doesn't look very good to me. Any suggestions or are you fine with that?

That said I also like @kantenkugel's idea to add it to the changelog command

Should I add it to the Changelog Command as well or should this be handled in another PR?

Edit:

Possibly needs to be expanded with month/year handling cuz i don't think i did those

Yes, you would. Only milliseconds to days are in the formatDuration function (so even the week one is missing).

The includeIntermed parameter determines what happens to 0-values... with includeIntermed set to true, 10h 0m 0s is possible. With includeIntermed set to false, it would just print 10h. 0s is only printed if no other value is >0

For this command, i would just include them... most of the time you won't have any 0-values anyway, so might as well go with all fields provided every time anyway.

For the changelog command, i would not add the duration btw. the datetime is enough there (timestamp of build)

For timezone, i would probably go with UTC.

Agree with Kant about UTC.

Looking at the last screenshot though I think it would be kinda cool to make it more text like, for example:

Version 3.8.3_464 was released on 22/06/2019 at 18:08 (GMT)
That was approximately 4 Days, 3 Hours and 32 Minutes ago.

Sure, mine was only an idea. You don't have to do it exactly like that.

Well normally I expect that the devs have the last word on how they want the code to be and not the people requesting their code to be merged. :D

Would I be allowed to make it public?

Any info on that? Then I can commit it real quick before going to bed and you can do further code checks. :D

I have never touched that piece of code but I don't see any reason why you could make it public

Alright, then I will make a copy and don't make it public.

*couldn't, I meant I don't see any reason why you couldn't make it public, sry

No worries, I already thought that, that's why I actually posted that comment. :D
And even if I would have used a copy, another commit and it's fixed so no worries. :)

Will be committed in the next 5 minutes.

…updated the Format of the printed DateTime Also added the Build Time to the ChangelogCommand and refactored the code a bit

DManstrator · 2019-07-02T23:25:25Z

bot/src/main/java/com/almightyalpaca/discord/jdabutler/commands/commands/ChangelogCommand.java

+            {
+                final JenkinsBuild build = args == null
+                        ? JENKINS.getLastSuccessfulBuild()
+                        : JENKINS.getBuild(Integer.parseInt(args[0]));


I believe I made a tiny mistake here. Shouldn't args[0] not be the item name according to line 45?

I mean VersionedItem#getChangelogProvider always returns null at the moment so it will always stop executing with "No Changelogs set up for " + item.getName() (line 57) but if this is gonna be changed, this will "crash" (the NumberFormatException will be catched) when providing another Item since args[0] isn't a build number / an integer in that case.

I would fix that by replacing args[0] with args[args.length - 1].

@kantenkugel I talked already with Alpaca in the JDA Server about this today, he has problems with the general code base right now so it would be great if you can also look over it (and maybe you can give me some response on that one too). However, it "sounds plausible" for Alpaca and it's ready to be merged (after I updated the code based on my last review).

DManstrator · 2019-07-10T13:20:07Z

bot/src/main/java/com/almightyalpaca/discord/jdabutler/util/DateUtils.java

+    public static final JenkinsApi JENKINS = JenkinsApi.JDA_JENKINS;
+    public static final DateTimeFormatter FORMATTER = getDateTimeFormatter();
+
+    public static DateTimeFormatter getDateTimeFormatter() {


The { should be in the next line to match the general code style.

DManstrator

JDA-Butler/bot/src/main/java/com/almightyalpaca/discord/jdabutler/util/DateUtils.java

Lines 17 to 26 in 6099bc7

    
           try 
        
           { 
        
               formatter = DateTimeFormatter.ofPattern(DATE_FORMAT + " (z)"); 
        
           } 
        
           catch (NullPointerException | IllegalArgumentException ex) 
        
           { 
        
               final String defaultFormat = "dd.MM.yyyy 'at' HH:mm:ss (z)"; 
        
               Bot.LOG.warn("Given format for DateVersionCommand was not valid, using: " + defaultFormat); 
        
               formatter = DateTimeFormatter.ofPattern(defaultFormat); 
        
           }

Maybe DateTimeFormatter.ofPattern should be called only once, doing the try-catch with a String (the pattern).

Edit:
Okay, don't know what I was thinking when commenting that. Adding (z) twice looked kinda ugly so I thought this could be made better, also then only one DateTimeFormatter.ofPattern would be needed but since there is no method to check if a pattern is valid, the try-catch is required. Or do you know how this can be done better?

Implementation of a Command printing the release time of a build

e9a5fba

kantenkugel requested changes Jun 27, 2019

View reviewed changes

Implemented changes requested by the Review

de55d6e

Added Logging for Exception occurrences, simplified getting a formatted time based of a OffsetDateTime and removed command name from List of Aliases

kantenkugel requested changes Jun 30, 2019

View reviewed changes

...src/main/java/com/almightyalpaca/discord/jdabutler/commands/commands/DateVersionCommand.java Outdated Show resolved Hide resolved

...src/main/java/com/almightyalpaca/discord/jdabutler/commands/commands/DateVersionCommand.java Outdated Show resolved Hide resolved

Correctly used the Logger class for logging an Exception

1691cd4

kantenkugel approved these changes Jun 30, 2019

View reviewed changes

Almighty-Alpaca reviewed Jul 1, 2019

View reviewed changes

Added UTC TimeZone and Duration Comparison to DateVersionCommand and …

6099bc7

…updated the Format of the printed DateTime Also added the Build Time to the ChangelogCommand and refactored the code a bit

DManstrator commented Jul 2, 2019

View reviewed changes

DManstrator commented Jul 10, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of a Command printing the release time of a build #47

Implementation of a Command printing the release time of a build #47

DManstrator commented Jun 26, 2019

kantenkugel left a comment

DManstrator commented Jun 27, 2019

DManstrator commented Jun 30, 2019

kantenkugel left a comment

kantenkugel commented Jun 30, 2019

DManstrator commented Jun 30, 2019

Almighty-Alpaca left a comment •

edited

Loading

Almighty-Alpaca Jul 1, 2019

kantenkugel Jul 1, 2019

DManstrator Jul 1, 2019 •

edited

Loading

kantenkugel Jul 2, 2019 •

edited

Loading

Almighty-Alpaca Jul 2, 2019

DManstrator Jul 2, 2019

Almighty-Alpaca Jul 2, 2019

DManstrator Jul 2, 2019

Almighty-Alpaca Jul 2, 2019

DManstrator Jul 2, 2019

DManstrator Jul 2, 2019 •

edited

Loading

DManstrator Aug 8, 2019

DManstrator Jul 10, 2019

DManstrator left a comment •

edited

Loading

	try
	{
	formatter = DateTimeFormatter.ofPattern(DATE_FORMAT + " (z)");
	}
	catch (NullPointerException \| IllegalArgumentException ex)
	{
	final String defaultFormat = "dd.MM.yyyy 'at' HH:mm:ss (z)";
	Bot.LOG.warn("Given format for DateVersionCommand was not valid, using: " + defaultFormat);
	formatter = DateTimeFormatter.ofPattern(defaultFormat);
	}

Implementation of a Command printing the release time of a build #47

Are you sure you want to change the base?

Implementation of a Command printing the release time of a build #47

Conversation

DManstrator commented Jun 26, 2019

Description

Example Preview

kantenkugel left a comment

Choose a reason for hiding this comment

DManstrator commented Jun 27, 2019

DManstrator commented Jun 30, 2019

kantenkugel left a comment

Choose a reason for hiding this comment

kantenkugel commented Jun 30, 2019

DManstrator commented Jun 30, 2019

Almighty-Alpaca left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DManstrator Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

kantenkugel Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DManstrator Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DManstrator left a comment • edited Loading

Choose a reason for hiding this comment

Almighty-Alpaca left a comment •

edited

Loading

DManstrator Jul 1, 2019 •

edited

Loading

kantenkugel Jul 2, 2019 •

edited

Loading

DManstrator Jul 2, 2019 •

edited

Loading

DManstrator left a comment •

edited

Loading