Skip to content
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

Issue #566 maxFiles doesn't seem to behave as expected #578

Merged
merged 3 commits into from
Mar 16, 2015

Conversation

nimrod-becker
Copy link
Contributor

Issue #566, when using maxFiles (with tailsble = false) deleting the oldest file (in _checkMaxFilesIncrementing) is not behaving as expected.
Calculating the target to delete, the log number should be (oldest !== 0 ? oldest : '') .

@indexzero
Copy link
Member

+1 @nimrod-becker could you add a test for this?

@nimrod-becker
Copy link
Contributor Author

Sure, actually there was a test especially for that case. However, the test always returned success (even though in fact there were 6 log files instead of three...).

I've fixed the test, verified it's failing with the original code of file transport and working with the fix.

} else {
// The other files should be exist
assert.doesNotThrow(function () {
fs.statSync(fullpath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: 2-space tabs here.

@nimrod-becker
Copy link
Contributor Author

Sorry about that :) fixed.

@indexzero
Copy link
Member

@nimrod-becker will check it out again today and merge it if all is kosher.

@nimrod-becker
Copy link
Contributor Author

any news ? :)

@indexzero
Copy link
Member

Works! 👍

indexzero added a commit that referenced this pull request Mar 16, 2015
Issue #566 maxFiles doesn't seem to behave as expected
@indexzero indexzero merged commit 8ba9be0 into winstonjs:master Mar 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants