New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1278 pull request for FileAppenderFactory #1561

Merged
merged 3 commits into from May 26, 2016

Conversation

Projects
None yet
3 participants
@pandaadb
Contributor

pandaadb commented May 26, 2016

Hi,

this pull request is with regards to:

FileAppenderFactory not valid for null currentLogFilename #1278

First time pull request, let me know if there is something I forgot.

This fix adds:

  • New validation based on the archived configuration to allow for null currentFileNames in case one wants to use the rolling policies file policy instead
  • Tests to assure that validation works and the rolling policy's filename is used instead.
@coveralls

This comment has been minimized.

coveralls commented May 26, 2016

Coverage Status

Coverage increased (+0.003%) to 81.152% when pulling 376d6a9 on pandaadb:1278-pull-request into b5fbc07 on dropwizard:master.

@NotNull
private String currentLogFilename;

private String currentLogFilename;

This comment has been minimized.

@nickbabcock

nickbabcock May 26, 2016

Contributor

Looks like you used a tab instead of spaces here. This issue also occurs in the tests. Please fix.

This comment has been minimized.

@pandaadb

pandaadb May 26, 2016

Contributor

Hi. yeah .. no idea why that happened. My eclipse is configured to use spaces but I guess Mac just ignores that and does whatever :/ I went through it manually and replaced it with spaces where I saw tabs.

}

@Test
public void testCurrentLogFileNameIsEmptyAndAppenderUsesArchivedNameInstead() throws Exception {

This comment has been minimized.

@nickbabcock

nickbabcock May 26, 2016

Contributor

How do you feel about the following implementation instead (it's a bit shorter):

@Test
public void testCurrentLogFileNameIsEmptyAndAppenderUsesArchivedNameInstead() throws Exception {
    final FileAppenderFactory<ILoggingEvent> appenderFactory = new FileAppenderFactory<>();
    appenderFactory.setArchivedLogFilenamePattern(folder.newFile("test-archived-name-%d.log").toString());
    final FileAppender<ILoggingEvent> rollingAppender = appenderFactory.buildAppender(new LoggerContext());

    final String file = rollingAppender.getFile();
    final String dateSuffix = LocalDateTime.now().format(DateTimeFormatter.ofPattern("YYYY-MM-dd"));
    final String name = Files.getNameWithoutExtension(file);
    Assert.assertEquals("test-archived-name-" + dateSuffix, name);
}

This comment has been minimized.

@pandaadb

pandaadb May 26, 2016

Contributor

yep, works for me :)

This comment has been minimized.

@nickbabcock

nickbabcock May 26, 2016

Contributor

Awesome, make the change and then this pull request looks good to me 👍

@coveralls

This comment has been minimized.

coveralls commented May 26, 2016

Coverage Status

Coverage increased (+0.003%) to 81.152% when pulling 274f13d on pandaadb:1278-pull-request into b5fbc07 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented May 26, 2016

Coverage Status

Coverage increased (+0.003%) to 81.152% when pulling f60425e on pandaadb:1278-pull-request into b5fbc07 on dropwizard:master.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 26, 2016

Looks good to me, thanks! 👍

@nickbabcock nickbabcock merged commit a9c3eec into dropwizard:master May 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment