Merge pull request #78 from log4js-node/fix-daysToKeep-alias-numBackups

fix: using options.numBackups instead of options.daysToKeep

README.md

@@ -55,7 +55,7 @@ When filename size >= maxSize then:
   * `compress` \<boolean\> - defaults to `false` - compress the backup files using gzip (files will have `.gz` extension).
   * `keepFileExt` \<boolean\> - defaults to `false` - keep the file original extension. e.g.: `abc.log -> abc.2013-08-30.log`.
   * `alwaysIncludePattern` \<boolean\> - defaults to `false` - extend the initial file with the pattern
-  * `daysToKeep` \<integer\> - defaults to `1` - the number of old files that matches the pattern to keep (excluding the hot file)
+  * <strike>`daysToKeep`</strike> `numBackups` \<integer\> - defaults to `1` - the number of old files that matches the pattern to keep (excluding the hot file)
 
 
 This returns a `WritableStream`. When the current time, formatted as `pattern`, changes then the current file will be renamed to `filename.formattedDate` where `formattedDate` is the result of processing the date through the pattern, and a new file will begin to be written. Streamroller uses [date-format](http://github.com/nomiddlename/date-format) to format dates, and the `pattern` should use the date-format format. e.g. with a `pattern` of `".yyyy-MM-dd"`, and assuming today is August 29, 2013 then writing to the stream today will just write to `filename`. At midnight (or more precisely, at the next file write after midnight), `filename` will be renamed to `filename.2013-08-29` and a new `filename` will be created. If `options.alwaysIncludePattern` is true, then the initial file will be `filename.2013-08-29` and no renaming will occur at midnight, but a new file will be written to with the name `filename.2013-08-30`.

lib/DateRollingFileStream.js

@@ -14,15 +14,20 @@ class DateRollingFileStream extends RollingFileWriteStream {
       pattern = 'yyyy-MM-dd';
     }
     options.pattern = pattern;
-    if (!options.daysToKeep && options.daysToKeep !== 0) {
-      options.daysToKeep = 1;
-    } else if (options.daysToKeep < 0) {
-      throw new Error(`options.daysToKeep (${options.daysToKeep}) should be >= 0`);
-    } else if (options.daysToKeep >= Number.MAX_SAFE_INTEGER) {
-      // to cater for numToKeep (include the hot file) at Number.MAX_SAFE_INTEGER
-      throw new Error(`options.daysToKeep (${options.daysToKeep}) should be < Number.MAX_SAFE_INTEGER`);
+    if (!options.numBackups && options.numBackups !== 0) {
+      if (!options.daysToKeep && options.daysToKeep !== 0) {
+        options.daysToKeep = 1;
+      } else {
+        process.emitWarning(
+          "options.daysToKeep is deprecated due the confusion it causes when used " + 
+          "together with file size rolling. Please use options.numBackups instead.",
+          "DeprecationWarning", "StreamRoller0001"
+        );
+      }
+      options.numBackups = options.daysToKeep;
+    } else {
+      options.daysToKeep = options.numBackups;
     }
-    options.numToKeep = options.daysToKeep + 1;
     super(filename, options);
     this.mode = this.options.mode;
   }

lib/RollingFileStream.js

@@ -9,17 +9,14 @@ class RollingFileStream extends RollingFileWriteStream {
     if (size) {
       options.maxSize = size;
     }
-    if (!backups && backups !== 0) {
-      backups = 1;
-    } else if (backups < 0) {
-      throw new Error(`backups (${backups}) should be >= 0`);
-    } else if (backups >= Number.MAX_SAFE_INTEGER) {
-      // to cater for numToKeep (include the hot file) at Number.MAX_SAFE_INTEGER
-      throw new Error(`backups (${backups}) should be < Number.MAX_SAFE_INTEGER`);
+    if (!options.numBackups && options.numBackups !== 0) {
+      if (!backups && backups !== 0) {
+        backups = 1;
+      }
+      options.numBackups = backups;
     }
-    options.numToKeep = backups + 1;
     super(filename, options);
-    this.backups = backups;
+    this.backups = options.numBackups;
     this.size = this.options.maxSize;
   }
 

lib/RollingFileWriteStream.js

@@ -104,7 +104,17 @@ class RollingFileWriteStream extends Writable {
     if (options.maxSize <= 0) {
       throw new Error(`options.maxSize (${options.maxSize}) should be > 0`);
     }
-    if (options.numToKeep <= 0) {
+    // options.numBackups will supercede options.numToKeep
+    if (options.numBackups || options.numBackups === 0) {
+      if (options.numBackups < 0) {
+        throw new Error(`options.numBackups (${options.numBackups}) should be >= 0`);
+      } else if (options.numBackups >= Number.MAX_SAFE_INTEGER) {
+        // to cater for numToKeep (include the hot file) at Number.MAX_SAFE_INTEGER
+        throw new Error(`options.numBackups (${options.numBackups}) should be < Number.MAX_SAFE_INTEGER`);
+      } else {
+        options.numToKeep = options.numBackups + 1;
+      }
+    } else if (options.numToKeep <= 0) {
       throw new Error(`options.numToKeep (${options.numToKeep}) should be > 0`);
     }
     debug(

test/DateRollingFileStream-test.js

@@ -460,48 +460,121 @@ describe("DateRollingFileStream", function() {
     });
   });
 
-  describe("with invalid number of daysToKeep", () => {
-    it("should complain about negative daysToKeep", () => {
-      const daysToKeep = -1;
+  describe("using deprecated daysToKeep", () => {
+    const onWarning = process.rawListeners("warning").shift();
+    let wrapper;
+    let stream;
+
+    before(done => {
+      const muteSelfDeprecation = (listener) => {
+        return (warning) => {
+          if (warning.name === "DeprecationWarning" && warning.code === "StreamRoller0001") {
+            return;
+          } else {
+            listener(warning);
+          }
+        };
+      };
+      wrapper = muteSelfDeprecation(onWarning);
+      process.prependListener("warning", wrapper);
+      process.off("warning", onWarning);
+      done();
+    });
+
+    after(async () => {
+      process.prependListener("warning", onWarning);
+      process.off("warning", wrapper);
+      await close(stream);
+      await remove(path.join(__dirname, "daysToKeep.log"));
+    });
+
+    it("should have deprecated warning for daysToKeep", () => {
+      process.on("warning", (warning) => {
+        warning.name.should.eql("DeprecationWarning");
+        warning.code.should.eql("StreamRoller0001");
+      });
+
+      stream = new DateRollingFileStream(
+        path.join(__dirname, "daysToKeep.log"),
+        { daysToKeep: 4 }
+      );
+    });
+
+    describe("with options.daysToKeep but not options.numBackups", () => {
+      it("should be routed from options.daysToKeep to options.numBackups", () => {
+        stream.options.numBackups.should.eql(stream.options.daysToKeep);
+      });
+
+      it("should be generated into stream.options.numToKeep from options.numBackups", () => {
+        stream.options.numToKeep.should.eql(stream.options.numBackups + 1);
+      });
+    });
+
+    describe("with both options.daysToKeep and options.numBackups", function() {
+      let stream;
+      it("should take options.numBackups to supercede options.daysToKeep", function() {
+        stream = new DateRollingFileStream(
+          path.join(__dirname, "numBackups.log"),
+          {
+            daysToKeep: 3,
+            numBackups: 9
+          }
+        );
+        stream.options.daysToKeep.should.not.eql(3);
+        stream.options.daysToKeep.should.eql(9);
+        stream.options.numBackups.should.eql(9);
+        stream.options.numToKeep.should.eql(10);
+      });
+
+      after(async function() {
+        await close(stream);
+        await remove("numBackups.log");
+      });
+    });
+  });
+
+  describe("with invalid number of numBackups", () => {
+    it("should complain about negative numBackups", () => {
+      const numBackups = -1;
       (() => {
         new DateRollingFileStream(
-          path.join(__dirname, "daysToKeep.log"),
-          { daysToKeep: daysToKeep }
+          path.join(__dirname, "numBackups.log"),
+          { numBackups: numBackups }
         );
-      }).should.throw(`options.daysToKeep (${daysToKeep}) should be >= 0`);
+      }).should.throw(`options.numBackups (${numBackups}) should be >= 0`);
     });
 
-    it("should complain about daysToKeep >= Number.MAX_SAFE_INTEGER", () => {
-      const daysToKeep = Number.MAX_SAFE_INTEGER;
+    it("should complain about numBackups >= Number.MAX_SAFE_INTEGER", () => {
+      const numBackups = Number.MAX_SAFE_INTEGER;
       (() => {
         new DateRollingFileStream(
-          path.join(__dirname, "daysToKeep.log"),
-          { daysToKeep: daysToKeep }
+          path.join(__dirname, "numBackups.log"),
+          { numBackups: numBackups }
         );
-      }).should.throw(`options.daysToKeep (${daysToKeep}) should be < Number.MAX_SAFE_INTEGER`);
+      }).should.throw(`options.numBackups (${numBackups}) should be < Number.MAX_SAFE_INTEGER`);
     });
   });
 
-  describe("with daysToKeep option", function() {
+  describe("with numBackups option", function() {
     let stream;
-    var daysToKeep = 4;
+    var numBackups = 4;
     var numOriginalLogs = 10;
 
     before(async function() {
       fakeNow = new Date(2012, 8, 13, 0, 10, 12); // pre-req to trigger a date-change later
       for (let i = 0; i < numOriginalLogs; i += 1) {
         await fs.writeFile(
-          path.join(__dirname, `daysToKeep.log.2012-09-${20-i}`), 
+          path.join(__dirname, `numBackups.log.2012-09-${20-i}`), 
           `Message on day ${i}\n`, 
           { encoding: "utf-8" }
         );
       }
       stream = new DateRollingFileStream(
-        path.join(__dirname, "daysToKeep.log"),
+        path.join(__dirname, "numBackups.log"),
         "yyyy-MM-dd",
         {
           alwaysIncludePattern: true,
-          daysToKeep: daysToKeep
+          numBackups: numBackups
         }
       );
     });
@@ -512,46 +585,46 @@ describe("DateRollingFileStream", function() {
         stream.write("Test message\n", "utf8", done);
       });
 
-      it("should be daysToKeep + 1 files left from numOriginalLogs", async function() {
+      it("should be numBackups + 1 files left from numOriginalLogs", async function() {
         const files = await fs.readdir(__dirname);
         var logFiles = files.filter(
-          file => file.indexOf("daysToKeep.log") > -1
+          file => file.indexOf("numBackups.log") > -1
         );
-        logFiles.should.have.length(daysToKeep + 1);
+        logFiles.should.have.length(numBackups + 1);
       });
     });
 
     after(async function() {
       await close(stream);
       const files = await fs.readdir(__dirname);
       const logFiles = files
-        .filter(file => file.indexOf("daysToKeep.log") > -1)
+        .filter(file => file.indexOf("numBackups.log") > -1)
         .map(f => remove(path.join(__dirname, f)));
       await Promise.all(logFiles);
     });
   });
 
-  describe("with daysToKeep and compress options", function() {
+  describe("with numBackups and compress options", function() {
     let stream;
-    const daysToKeep = 4;
+    const numBackups = 4;
     const numOriginalLogs = 10;
 
     before(async function() {
       fakeNow = new Date(2012, 8, 13, 0, 10, 12); // pre-req to trigger a date-change later
       for (let i = numOriginalLogs; i >= 0; i -= 1) {
         const contents = await gzip(`Message on day ${i}\n`);
         await fs.writeFile(
-          path.join(__dirname, `compressedDaysToKeep.log.2012-09-${20-i}.gz`),
+          path.join(__dirname, `compressedNumBackups.log.2012-09-${20-i}.gz`),
           contents
         );
       }
       stream = new DateRollingFileStream(
-        path.join(__dirname, "compressedDaysToKeep.log"),
+        path.join(__dirname, "compressedNumBackups.log"),
         "yyyy-MM-dd",
         {
           alwaysIncludePattern: true,
           compress: true,
-          daysToKeep: daysToKeep
+          numBackups: numBackups
         }
       );
     });
@@ -565,17 +638,17 @@ describe("DateRollingFileStream", function() {
       it("should be 5 files left from original 11", async function() {
         const files = await fs.readdir(__dirname);
         var logFiles = files.filter(
-          file => file.indexOf("compressedDaysToKeep.log") > -1
+          file => file.indexOf("compressedNumBackups.log") > -1
         );
-        logFiles.should.have.length(daysToKeep + 1);
+        logFiles.should.have.length(numBackups + 1);
       });
     });
 
     after(async function() {
       await close(stream);
       const files = await fs.readdir(__dirname);
       const logFiles = files
-        .filter(file => file.indexOf("compressedDaysToKeep.log") > -1)
+        .filter(file => file.indexOf("compressedNumBackups.log") > -1)
         .map(f => remove(path.join(__dirname, f)));
       await Promise.all(logFiles);
     });

test/RollingFileStream-test.js

@@ -128,7 +128,7 @@ describe("RollingFileStream", function() {
           1024,
           backups
         );
-      }).should.throw(`backups (${backups}) should be >= 0`);
+      }).should.throw(`options.numBackups (${backups}) should be >= 0`);
     });
 
     it("should complain about backups >= Number.MAX_SAFE_INTEGER", () => {
@@ -139,7 +139,51 @@ describe("RollingFileStream", function() {
           1024,
           backups
         );
-      }).should.throw(`backups (${backups}) should be < Number.MAX_SAFE_INTEGER`);
+      }).should.throw(`options.numBackups (${backups}) should be < Number.MAX_SAFE_INTEGER`);
+    });
+  });
+
+  describe("using backups", () => {
+    describe("with backups but not options.numBackups", () => {
+      let stream;
+      it("should be routed from backups to options.numBackups", function() {
+        stream = new RollingFileStream(
+          path.join(__dirname, "test-rolling-file-stream"),
+          1024,
+          3
+        );
+        stream.options.numBackups.should.eql(stream.backups);
+      });
+
+      it("should be generated into stream.options.numToKeep from options.numBackups", function() {
+        stream.options.numToKeep.should.eql(stream.options.numBackups + 1);
+      });
+
+      after(async function() {
+        await close(stream);
+        await remove("test-rolling-file-stream");
+      });
+    });
+
+    describe("with both backups and options.numBackups", function() {
+      let stream;
+      it("should take options.numBackups to supercede backups", function() {
+        stream = new RollingFileStream(
+          path.join(__dirname, "test-rolling-file-stream"),
+          1024,
+          3,
+          { numBackups: 9 }
+        );
+        stream.backups.should.not.eql(3);
+        stream.backups.should.eql(9);
+        stream.options.numBackups.should.eql(9);
+        stream.options.numToKeep.should.eql(10);
+      });
+
+      after(async function() {
+        await close(stream);
+        await remove("test-rolling-file-stream");
+      });
     });
   });