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

Don't log if logging is disabled #50

Merged
merged 1 commit into from
Jun 3, 2014
Merged

Don't log if logging is disabled #50

merged 1 commit into from
Jun 3, 2014

Conversation

rlivsey
Copy link
Contributor

@rlivsey rlivsey commented Jun 3, 2014

Previously this guard was in the logging method itself, should it be moved back there instead?

@xtian
Copy link
Contributor

xtian commented Jun 3, 2014

Running into this at the moment

@stefanpenner
Copy link
Contributor

tests please

@rlivsey
Copy link
Contributor Author

rlivsey commented Jun 3, 2014

@stefanpenner on their way, would you prefer the guard in the _logLookup method or around each call as it is now?

@stefanpenner
Copy link
Contributor

@rlivsey all of logLookup should be guarded.

@rlivsey
Copy link
Contributor Author

rlivsey commented Jun 3, 2014

Ok, updated & added some tests. loggingDisabled isn't passed to _logLookup so I've checked for that separately which seemed to make the most sense.

The tests just check to see whether anything was logged at all, not their content, which I hope is sufficient for this specific pull request.

@@ -200,12 +200,14 @@ define("ember/resolver",
}

if (tmpModuleName && moduleEntries[tmpModuleName]) {
self._logLookup(true, parsedName, tmpModuleName);
if (!loggingDisabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where does findModuleName get called other then lookupDescription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up in resolveOther

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

stefanpenner added a commit that referenced this pull request Jun 3, 2014
Don't log if logging is disabled
@stefanpenner stefanpenner merged commit c48e5bf into ember-cli:master Jun 3, 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.

3 participants