Skip to content

Log levels #590

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

Merged
merged 7 commits into from
Jun 2, 2016
Merged

Log levels #590

merged 7 commits into from
Jun 2, 2016

Conversation

mdtusz
Copy link
Contributor

@mdtusz mdtusz commented May 31, 2016

Resolves #565.

In brief:

  • remove Lib.markTime calls - these can be done using console.time and console.timeEnd while developing, then should be removed before committing/pushing
  • Add Lib.log, Lib.warn, and Lib.error which can be toggled by setting the logging config value
Logging level Result
`0
1 Only warnings and errors will be output
2 Warnings, errors, and log messages will be output

There is a downside: the line number when logged shows up as from the lib/index.js file. I have somewhat "fixed" this by having stack traces printed with logs, but it is not ideal (in my mind) and the methods for circumventing this aren't very robust and don't work with strict mode (using arguments.caller.callee or overwriting Error.prepareStackTrace).

I think any downsides (i.e. having to click to see the stacktrace on a log message) is worth the benefits of keeping the library quiet in a production setting.

@@ -9,6 +9,7 @@
"Uint8Array": true
},
"rules": {
"strict": [2, "global"]
"strict": [2, "global"],
"no-console": [1]
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this a [2].

};

loggers.error = function() {
if(config.logging > 0) {
Copy link
Contributor

@n-riesco n-riesco Jun 1, 2016

Choose a reason for hiding this comment

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

No prefix 'ERROR:'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was mainly because console.error marks it red as an error. Should I add it for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the prefix would be useful for users that want to capture the console and process the messages.

@etpinard
Copy link
Contributor

etpinard commented Jun 2, 2016

💃

@mdtusz mdtusz merged commit b4785b0 into master Jun 2, 2016
@mdtusz mdtusz deleted the log-levels branch June 2, 2016 14:39
**It is important to note that logging is turned off by default in v1.13.0 onwards.**
To turn logging on for development, you will want to run
`Plotly.setPlotConfig({ logging: 2 })` before any plotting.
See [this file](https://github.com/plotly/plotly.js/blob/master/src/lib/loggers.js) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I'd vote for leaving this block up until September and then remove it.

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.

Don't output console.logs in production (don't use console.log directly)
3 participants