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

Logging module #1405

Closed
wants to merge 2 commits into from
Closed

Logging module #1405

wants to merge 2 commits into from

Conversation

ryanrolds
Copy link
Contributor

@ryanrolds ryanrolds commented Jan 30, 2017

Adds logging module discussed in #1390.

@@ -0,0 +1,78 @@
define(['core/js/adapt'], function(Adapt) {
Copy link
Member

Choose a reason for hiding this comment

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

could you put some whitespace into this file?

should be spaces 4 indentation, line breaks between function definitions etc

Copy link
Contributor Author

@ryanrolds ryanrolds Feb 14, 2017

Choose a reason for hiding this comment

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

The 4 for indentation conflicts with the 2 stated in the style guide which is linked to on the the Contributing-code page.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore the referenced/copied standard, it hasn't been formalized yet. It should say "spaces 4" indentation, no tabs, etc.

this.debug('Logging config loaded');
this.trigger('log:ready');
},
debug: function(msg, data) {
Copy link
Member

@oliverfoster oliverfoster Feb 10, 2017

Choose a reason for hiding this comment

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

These functions need their parameters to behaviour more like the console.log function.
essentially each argument forms part of the output string.

    fatal: function() {
      this._log('fatal', [].array.slice.call(arguments, 0)));
    },
    _log: function(level, datas) {
      if (!this._config._isEnabled) {
        return;
      }

      if (this._levels[level] < this._levels[this._config._level]) {
        return;
      }

      if (this._config._console) {
        var log = [level.toUpperCase() + ':'];
        datas && log.push.apply(log, datas);

        console.log.apply(console, log);
      }

      // Allow error reporting plugins to hook and report to logging systems
      this.trigger('log:' + level, datas);
    }

Allowing:

Adapt.log.fatal("this", "is", "a", "fatal", "error");
//FATAL: this is a fatal 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.

Will do.

@ryanrolds
Copy link
Contributor Author

  • Fixed the indentation
  • Adjusted log function arguments
  • Query string override is now case-insensitive
  • Added some blank lines
  • Comments

Should be ready to review again.

Copy link
Member

@oliverfoster oliverfoster left a comment

Choose a reason for hiding this comment

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

Beautiful ta. Much easier to read. It needs to work in ie8 though, so you'll need to use underscore bind and supply arguments to your splice calls, I think.

Also, only if you fancy doing it. You can return early twice in the initialize function, rather than a double nested if. Invert the expressions and return, flattening the code.

@ryanrolds
Copy link
Contributor Author

ryanrolds commented Feb 14, 2017

IE8 has been EOL for a while now and as far as I'm aware is not supported on any current MS OSs.

@ryanrolds
Copy link
Contributor Author

For ethical reasons I don't make changes to specifically support IE8, the browser is EOL and no longer receiving security updates, it's usage is negligent.

@oliverfoster
Copy link
Member

Fair enough. I'll see if someone else can take over.

@oliverfoster oliverfoster mentioned this pull request Feb 16, 2017
@oliverfoster
Copy link
Member

functionality added on #1425

thanks for doing this @ryanrolds
nice bit of code

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.

2 participants