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

Options should be optional #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

melbourne2991
Copy link

Currently if you do not provide options you will get "cannot get property level of undefined"

Currently if you do not provide options you will get "cannot get property level of undefined"
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.617% when pulling acf3fd1 on melbourne2991:patch-1 into ece0847 on lazywithclass:master.

@lazywithclass
Copy link
Owner

Could you please provide a code example showing in which case that error is shown?

@melbourne2991
Copy link
Author

winston.add(WinstonCloudWatch);

@lazywithclass
Copy link
Owner

I see. Then I don't think the fix is to let someone add to winston a configuration that will surely not work, because it doesn't have any information on where to store logs.

What's your use case? Why would you need to be able to use this library without, for example, a log stream and a log group?

@melbourne2991
Copy link
Author

If you don't allow an empty object it fails but the error message is unclear and not relevant to the actual problem - "cannot get property level of undefined", when you do allow an empty object the code will fail when it tries to get logGroupName and the error is more relevant (something along the lines of "logGroupName" not defined). I was confused about "level" being undefined because the Readme says level defaults to "info". So I spent quite a bit of time trying to figure out what was wrong with my "level" when really the problem was that I was missing logGroupName and logStreamName.

@lazywithclass
Copy link
Owner

Cool, looks like we're talking about validation. Thanks for the details, they're really helpful for me to understand what the problem is.

I think I would like to do some basic validation on the options, so that we check that at least logGroup and logStream are there.

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.

3 participants