-
Notifications
You must be signed in to change notification settings - Fork 110
elm logger #47
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.
Style: A little too much var.
|
is elm ready yet? at least is it close to entering the review procedure? |
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.
Need to add [NotNull] in lots of places. Don't need to add it if DI is constructing the object, but in cases where a developer is calling the API we need to add guards.
|
@borgdylan hopefully soon! it's in the review process |
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 think both Guid and LogLevel are structs and so cannot be null. (Hopefully when we eventually implement the NotNull stuff it'll throw under such usage.)
|
@davidfowl updated |
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.
context.Request.Query["level"]
|
Cool. I think this is |
|
@sonjakhan I am trying to use elm but I am getting exceptions that the Form could not be read. Why should there be a form if i am accessing /Elm using the browser address bar directly? |
|
@borgdylan can you log a bug for this? Sonja was an intern and isn't currently working on ELM (though we welcome any future contributions from her! - Hi, @sonjakhan !). |
moved this from the Logging repo (aspnet/Logging#38), removed the Mvc dependency from the sample, and fixed scoping