fix(): Normalize options.maxSize and throw errors where appropriate #397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The documentation does not make it very clear the difference between passing an integer and a string for maxSize, there also seems to be quite a bit of confusion around whether or not to use `k` or `kb` but there is no reason that `k`, `kb` and `kilobytes` can't all be used interchangeably (as an example). This pull request starts off by renaming the `getMaxSize` function to something more appropriate `normalizeMaxSize`, it also updates the `size` option that is provided to `getStream` to determine more quickly if `maxSize` is undefined, and if so go ahead and just send `null`. Next it normalizes the `maxSize` option and determines if it can return more quickly based upon the type of the data that is provided. Such that if an integer is provided, do a small check and bit of math and return it, if a string is provided, check to see if it was sent in an already correctly formatted quantifier and return it, if a longer quantifier was used, go ahead and parse it and assign the correct quantifier for passing it to the file-stream-rotator package. It also now throws errors in the appropriate places if the provided option can not be parsed instead of just silently passing null to the file-stream-rotator package and confusing users.
|
@mattberther @indexzero Would one of you mind taking a look at this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The documentation does not make it very clear the difference between passing an integer and a string for maxSize, there also seems to be quite a bit of confusion around whether or not to use
korkbbut there is no reason thatk,kbandkilobytescan't all be used interchangeably (as an example). It alsofailssilently by just passingnullto the file-stream-rotator package instead of letting the user know that they did something wrong and their option won't be used.It also removes the silent rounding of the maxSize option that was happening, not sure why it was doing that, if users pass in a specific number, there is probably a reason for it.
This pull request starts off by renaming the
getMaxSizefunction to something more appropriatenormalizeMaxSize, it also updates thesizeoption that is provided togetStreamto determine more quickly ifmaxSizeis undefined, and if so go ahead and just sendnull.Next it normalizes the
maxSizeoption and determines if it can return more quickly based upon the type of the data that is provided. Such that if an integer is provided, do a small check and bit of math and return it, if a string is provided, check to see if it was sent in an already correctly formatted quantifier and return it, if a longer quantifier was used, go ahead and parse it and assign the correct quantifier for passing it to the file-stream-rotator package.It also now throws errors in the appropriate places if the provided option can not be parsed instead of just silently passing null to the file-stream-rotator package and confusing users.
I have not made any changes to the tests and as far as I can tell this change is not breaking and should not affect anyone in a negative way (unless they were passing in a crazy maxSize that was being ignored anyways) and this should also prevent issues being opened like #387