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

Add categories for Y.log entries missing them #1610

Merged
merged 1 commit into from
Feb 16, 2014

Conversation

andrewnicols
Copy link
Contributor

As discussed with @juandopazo

@juandopazo
Copy link
Member

It seems that we really want to change Y.log so that it defaults to either debug or info, right?

@andrewnicols
Copy link
Contributor Author

Okay, I've modified the patch so that we set a default category on the condition that:

  • no category was specified; or
  • a category not specified in the list of valid log levels.

I looked at changing the default logLevel to 'warn' (from debug) but this causes all unit tests to not log so needs more of a look in, perhaps in another issue.

@andrewnicols
Copy link
Contributor Author

Looks like the fail is unrelated to these changes.

@caridy
Copy link
Member

caridy commented Feb 5, 2014

I'm not convinced that just adding a new default category for log messages is going to help.

Obviously, bunch of people will do Y.log('msg') without a category, and that doesn't means it is debug, it is probably info.

@andrewnicols
Copy link
Contributor Author

At the moment, there are too many instances of Y.log which seem to have
been left left lying around, either with no log category defined or an
invalid one. This is either because the signature has been misread and the
src has been used as a category, or because a new one was made up (e.g.
much of base using 'life'). They're making all logging pretty much useless
so anything to reduce the number of messages is an improvement.

I'd argue that most people don't really want to read these messages and if
they really are intended as 'info' level, then they should be actively
specified as having an info category.

That said, if you'd prefer this to go down as 'info', so I'm happy to
adjust the patch if that's the general agreement amongst others.

We'd very much like this to make the next release of YUI so that we can
include it in Moodle before our impending code freeze so that we can
actually catch deprecation notices and log in a sane fashion.

Cheers,

Andrew

On 6 February 2014 00:00, Caridy Patino notifications@github.com wrote:

I'm not convinced that just adding a new default category for log messages
is going to help.

Obviously, bunch of people will do Y.log('msg') without a category, and
that doesn't means it is debug, it is probably info.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1610#issuecomment-34195939
.

@juandopazo
Copy link
Member

+1 to info.

Having a default category helps. But the next step is to default Y.config.logLevel to error or warn. Not sure which.

@caridy
Copy link
Member

caridy commented Feb 5, 2014

Yeah, default to info and logLevel to warn will be good enough I think.

@andrewnicols
Copy link
Contributor Author

Thanks guys,

I've updated the default log category to 'info'.

With regards the default Y.config.logLevel, I'm not sure that this is something I best know how to do.
The default value is defined at https://github.com/yui/yui3/blob/dev-master/src/yui/js/yui-log.js#L58 and is easy enough to change, but has the result that all unit tests fail to display their output through the console.

Obviously, we could go through and update all unit tests to specify a Y.config.logLevel of 'debug', either through use of YUI_config, or as a set of params to every YUI() instantiation, but this won't help third party users. The tests will still run through Yeti and provide results, but they won't be visible in a browser.

Thoughts?

Set a default log category of info if no category was specified, or the
category specified is not in the list of valid categories.
@caridy
Copy link
Member

caridy commented Feb 6, 2014

thanks @andrewnicols this looks good!

@juandopazo
Copy link
Member

I'll look into the tests issue later today.

@andrewnicols
Copy link
Contributor Author

Any chance of getting this section integrated before the pull freeze on Friday? I think that changing the default logLevel should be considered as a separate issue anyway.

@clarle
Copy link
Collaborator

clarle commented Feb 16, 2014

I agree with @andrewnicols and this looks fine to me. I'll open the default logLevel issue separately after this gets in.

@clarle clarle merged commit dacc8fe into yui:dev-master Feb 16, 2014
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.

6 participants