-
Notifications
You must be signed in to change notification settings - Fork 469
Add verbose warn and error log methods to node #1058
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm finding the following code very difficult to understand/read. Maybe its just me, but its making my head hurt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair, a more C# style is probably better for maintainability
basic idea was 'log' is an object of tracewriter methods. We need to decorate those methods with the util.format code and then get it into a format where context.log is the info tracewriter with all the tracewriter methods attached to it as properties (C# doesn't seems to have the idea of callable objects (I guess it would be extending Func
?), which is what this is mapping to)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that you could construct the set of log methods in this code, but have them all funnel down into a single log call into the func exposed from .NET - just passing in an object of the form { level: 'info', message: '...' }. That allows us to just have a single simple method in .NET receiving all of these. For example:
var origLog = context.log;
var logLevel = function (level) {
return function () {
var message = util.format.apply(null, args);
origLog({ level: level, message: message});
};
}
var levels = ['info', 'verbose', 'warn', 'error' ];
var log = logLevel('info');
levels.foreach(level => {
log[level] = logLevel(level);
});
context.log = log;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this would all be much simpler if we just make the existing log be a function that takes and object like { level: 'info', message: '...' }. Our node adapter code then passes in such objects, and we map like:
// create a TraceWriter wrapper that can be exposed to Node.js
var log = (Func<object, Task<object>>)(p =>
{
var logData = (IDictionary<string, object>)p;
string message = (string)logData["message"];
if (message != null)
{
try
{
TraceLevel level = (TraceLevel)Enum.Parse(typeof(TraceLevel), (string)logData["level"]);
var evt = new TraceEvent(level, message);
traceWriter.Trace(evt);
}
catch (ObjectDisposedException)
{
// if a function attempts to write to a disposed
// TraceWriter. Might happen if a function tries to
// log after calling done()
}
}
return Task.FromResult<object>(null);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy paste error? Anyhow, I think you should just add your new cases to the existing logging scenario script and test. simpler that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with my suggestion above, this change shouldn't be needed, right? since log remains a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
30ecf17
to
e303db4
Compare
resolves #304