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

Winston 3 compatibility #39

Closed
wants to merge 7 commits into from
Closed

Winston 3 compatibility #39

wants to merge 7 commits into from

Conversation

@OtterCode
Copy link

@OtterCode OtterCode commented Jun 22, 2018

No description provided.

@@ -129,7 +141,11 @@ Loggly.prototype.log = function (level, msg, meta, callback) {
callback(err, true);
}

<<<<<<< HEAD

This comment has been minimized.

@mostlyjason

mostlyjason Jun 25, 2018

looks like there might be an merge conflict you need to resolve?

@@ -139,9 +155,9 @@ Loggly.prototype.log = function (level, msg, meta, callback) {
// #### @options {Object} Set stream options
// Returns a log stream.
//
Loggly.prototype.stream = function(options) {
Loggly.prototype.stream = function(maybeOptions) {

This comment has been minimized.

@mostlyjason

mostlyjason Jun 25, 2018

why rename to maybeOptions?

This comment has been minimized.

@OtterCode

OtterCode Jun 25, 2018
Author

maybeOptions is just a personal convention for function arguments that can be undefined. Later in the function on line 160 it assigns options to either the argument that gets passed, or an empty object, if it's undefined. Reassignment to the same variable is good to avoid wherever possible . The raw maybeOptions argument doesn't get used until you later guarantee that it contains at least an empty object, and two variables is the best way to that.

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jun 25, 2018

@OtterCode will this new version be backward-compatible with Winston 2.x or do we need to keep it separate?

@OtterCode
Copy link
Author

@OtterCode OtterCode commented Jun 25, 2018

@mostlyjason So, I changed the winston version to 2.4 in the package, and tests still passed. I've got a few small tweaks to push, then it should all be good. We should still probably merge it into the 3.x branch first, just for verification.

@OtterCode OtterCode closed this Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.