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

Added support for -log parameter using log4net. Writing additional messages to log. #135

Merged
merged 19 commits into from Apr 29, 2013

Conversation

Projects
None yet
8 participants
@dschenkelman
Contributor

dschenkelman commented Mar 16, 2013

Feature for #82

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 21, 2013

Member

do you mind pulling the latest version of dev into this? Thanks!

Member

filipw commented Mar 21, 2013

do you mind pulling the latest version of dev into this? Thanks!

Merge branch 'dev' into loglevels
Conflicts:
	src/ScriptCs/CompositionRoot.cs
	src/ScriptCs/Program.cs
	src/ScriptCs/ScriptCsArgs.cs
@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Mar 21, 2013

Contributor

@filipw Just did. Let me know if there are any issues.

Contributor

dschenkelman commented Mar 21, 2013

@filipw Just did. Let me know if there are any issues.

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 22, 2013

Member

Thanks a lot. I have a few comments:

I don't feel there is any good reason for taking dependency on log4net in the core libraries. Now it doesn't do anything there except provide an ILog interface and log levels. So all users of core/roslyn have to pull this library for no real reason;

I'd prefer that we have an abstraction (ILogWriter or something along these lines) in the core. I actually understood that was the agreement in the discussion in #82

scriptcs.exe could still use log4net implementation to provide a log writer (exactly like it does now, except over this new abstraction).

Member

filipw commented Mar 22, 2013

Thanks a lot. I have a few comments:

I don't feel there is any good reason for taking dependency on log4net in the core libraries. Now it doesn't do anything there except provide an ILog interface and log levels. So all users of core/roslyn have to pull this library for no real reason;

I'd prefer that we have an abstraction (ILogWriter or something along these lines) in the core. I actually understood that was the agreement in the discussion in #82

scriptcs.exe could still use log4net implementation to provide a log writer (exactly like it does now, except over this new abstraction).

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 22, 2013

Member

It would be great if you could change that!

Member

filipw commented Mar 22, 2013

It would be great if you could change that!

@jrusbatch

This comment has been minimized.

Show comment
Hide comment
@jrusbatch

jrusbatch Mar 22, 2013

Member

I share @filipw's views on taking a dependency on log4net in the core library. I think the less dependencies Core/Contract/Engines have, the better.

Member

jrusbatch commented Mar 22, 2013

I share @filipw's views on taking a dependency on log4net in the core library. I think the less dependencies Core/Contract/Engines have, the better.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as it
is swappable. I'd much rather that then creating a new logger / log levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch
notifications@github.comwrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299
.

Member

glennblock commented Mar 22, 2013

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as it
is swappable. I'd much rather that then creating a new logger / log levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch
notifications@github.comwrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299
.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

So how about we do this.

  1. We create a logger abstraction.
  2. We have a default implementation of that in the box. I don't see any
    issue in that being log4net but I am open to others thoughts.

On Fri, Mar 22, 2013 at 3:09 AM, Glenn Block glenn.block@gmail.com wrote:

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as it
is swappable. I'd much rather that then creating a new logger / log levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch <
notifications@github.com> wrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299
.

Member

glennblock commented Mar 22, 2013

So how about we do this.

  1. We create a logger abstraction.
  2. We have a default implementation of that in the box. I don't see any
    issue in that being log4net but I am open to others thoughts.

On Fri, Mar 22, 2013 at 3:09 AM, Glenn Block glenn.block@gmail.com wrote:

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as it
is swappable. I'd much rather that then creating a new logger / log levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch <
notifications@github.com> wrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299
.

@nberardi

This comment has been minimized.

Show comment
Hide comment
@nberardi

nberardi Mar 22, 2013

Why not use this http://nuget.org/packages/Common.Logging instead of
creating your own.

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On Mar 22, 2013, at 3:10 AM, Glenn Block notifications@github.com wrote:

So how about we do this.

  1. We create a logger abstraction.
  2. We have a default implementation of that in the box. I don't see any
    issue in that being log4net but I am open to others thoughts.

On Fri, Mar 22, 2013 at 3:09 AM, Glenn Block glenn.block@gmail.com wrote:

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as
it
is swappable. I'd much rather that then creating a new logger / log
levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch <
notifications@github.com> wrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299>
.


Reply to this email directly or view it on
GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15283905
.

nberardi commented Mar 22, 2013

Why not use this http://nuget.org/packages/Common.Logging instead of
creating your own.

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On Mar 22, 2013, at 3:10 AM, Glenn Block notifications@github.com wrote:

So how about we do this.

  1. We create a logger abstraction.
  2. We have a default implementation of that in the box. I don't see any
    issue in that being log4net but I am open to others thoughts.

On Fri, Mar 22, 2013 at 3:09 AM, Glenn Block glenn.block@gmail.com wrote:

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as
it
is swappable. I'd much rather that then creating a new logger / log
levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch <
notifications@github.com> wrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299>
.


Reply to this email directly or view it on
GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15283905
.

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 22, 2013

Member

@glennblock that's what I suggested too. Except I'd see log4net impl contained within scriptcs CLI project and injected as a depndency over the abstraction.

And if we feel very strongly about having at least one concrete implementation in the core, I'd vote for having it against System.Diagnostics.Trace - because:
a) it's dependency free
b) any logging solution (nlog, log4net etc) provide you trace listeners so if you use scriptcs.core and want to use something like log4net in your app then either provide a simple adapter to our abstraction, or you just use log4net trace listener

Member

filipw commented Mar 22, 2013

@glennblock that's what I suggested too. Except I'd see log4net impl contained within scriptcs CLI project and injected as a depndency over the abstraction.

And if we feel very strongly about having at least one concrete implementation in the core, I'd vote for having it against System.Diagnostics.Trace - because:
a) it's dependency free
b) any logging solution (nlog, log4net etc) provide you trace listeners so if you use scriptcs.core and want to use something like log4net in your app then either provide a simple adapter to our abstraction, or you just use log4net trace listener

@jrusbatch

This comment has been minimized.

Show comment
Hide comment
@jrusbatch

jrusbatch Mar 22, 2013

Member

👍 for using System.Diagnostics.Trace. That would eliminate the need for us to provide (expose) our own logging interface, as well. Instead the consumer can register their own ITraceListener and configure tracing verbosity through their app's configuration file.

Member

jrusbatch commented Mar 22, 2013

👍 for using System.Diagnostics.Trace. That would eliminate the need for us to provide (expose) our own logging interface, as well. Instead the consumer can register their own ITraceListener and configure tracing verbosity through their app's configuration file.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

Ok, but in that case scriptscs.core should have it so that scriptcs can reference it to inject it. Or it should be in a 3rd project which scriptcs references and which a hosted can reference if they use scriptcs.core.

I don't particularly care which of the two approaches as long as we make it avail and don't reimplement our own logger.

-----Original Message-----
From: "Filip W" notifications@github.com
Sent: ‎3/‎22/‎2013 8:29 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

@glennblock that's what I suggested too. Except I'd see log4net impl contained within scriptcs CLI project and injected as a depndency over the abstraction.
And if we feel very strongly about having at least one concrete implementation in the core, I'd vote for having it against System.Diagnostics.Trace - because:
a) it's dependency free
b) any logging solution (nlog, log4net etc) provide you trace listeners so if you use scriptcs.core and want to use something like log4net in your app then either provide a simple adapter to our abstraction, or you just use log4net trace listener

Reply to this email directly or view it on GitHub.

Member

glennblock commented Mar 22, 2013

Ok, but in that case scriptscs.core should have it so that scriptcs can reference it to inject it. Or it should be in a 3rd project which scriptcs references and which a hosted can reference if they use scriptcs.core.

I don't particularly care which of the two approaches as long as we make it avail and don't reimplement our own logger.

-----Original Message-----
From: "Filip W" notifications@github.com
Sent: ‎3/‎22/‎2013 8:29 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

@glennblock that's what I suggested too. Except I'd see log4net impl contained within scriptcs CLI project and injected as a depndency over the abstraction.
And if we feel very strongly about having at least one concrete implementation in the core, I'd vote for having it against System.Diagnostics.Trace - because:
a) it's dependency free
b) any logging solution (nlog, log4net etc) provide you trace listeners so if you use scriptcs.core and want to use something like log4net in your app then either provide a simple adapter to our abstraction, or you just use log4net trace listener

Reply to this email directly or view it on GitHub.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

Great idea!

-----Original Message-----
From: "Nick Berardi" notifications@github.com
Sent: ‎3/‎22/‎2013 7:41 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

Why not use this http://nuget.org/packages/Common.Logging instead of
creating your own.

Nick Berardi
Sent on the go from my phone.

On Mar 22, 2013, at 3:10 AM, Glenn Block notifications@github.com wrote:

So how about we do this.

  1. We create a logger abstraction.
  2. We have a default implementation of that in the box. I don't see any
    issue in that being log4net but I am open to others thoughts.

On Fri, Mar 22, 2013 at 3:09 AM, Glenn Block glenn.block@gmail.com wrote:

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as
it
is swappable. I'd much rather that then creating a new logger / log
levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch <
notifications@github.com> wrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299>
.


Reply to this email directly or view it on
GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15283905
.

Reply to this email directly or view it on GitHub.

Member

glennblock commented Mar 22, 2013

Great idea!

-----Original Message-----
From: "Nick Berardi" notifications@github.com
Sent: ‎3/‎22/‎2013 7:41 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

Why not use this http://nuget.org/packages/Common.Logging instead of
creating your own.

Nick Berardi
Sent on the go from my phone.

On Mar 22, 2013, at 3:10 AM, Glenn Block notifications@github.com wrote:

So how about we do this.

  1. We create a logger abstraction.
  2. We have a default implementation of that in the box. I don't see any
    issue in that being log4net but I am open to others thoughts.

On Fri, Mar 22, 2013 at 3:09 AM, Glenn Block glenn.block@gmail.com wrote:

I was the one who recommended log4net as we need a default. I agree we
could wrap it, but I don't see a harm in having it by default as long as
it
is swappable. I'd much rather that then creating a new logger / log
levels
implementation just for scriptcs.

On Thu, Mar 21, 2013 at 10:41 PM, Justin Rusbatch <
notifications@github.com> wrote:

I share @filipw https://github.com/filipw's views on taking a
dependency on log4net in the core library. I think the less dependencies
Core/Contract/Engines have, the better in my opinion.


Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/pull/135#issuecomment-15278299>
.


Reply to this email directly or view it on
GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15283905
.

Reply to this email directly or view it on GitHub.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

Let's not do that.

-----Original Message-----
From: "Justin Rusbatch" notifications@github.com
Sent: ‎3/‎22/‎2013 8:59 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

for using System.Diagnostics.Trace. That would eliminate the need for us to provide (expose) our own logging interface, as well. Instead the consumer can register their own ITraceListener and configure tracing verbosity through their app's configuration file.

Reply to this email directly or view it on GitHub.

Member

glennblock commented Mar 22, 2013

Let's not do that.

-----Original Message-----
From: "Justin Rusbatch" notifications@github.com
Sent: ‎3/‎22/‎2013 8:59 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

for using System.Diagnostics.Trace. That would eliminate the need for us to provide (expose) our own logging interface, as well. Instead the consumer can register their own ITraceListener and configure tracing verbosity through their app's configuration file.

Reply to this email directly or view it on GitHub.

@nberardi

This comment has been minimized.

Show comment
Hide comment
@nberardi

nberardi Mar 22, 2013

@glennblock Common.Logging is a great library, I have used it on a number of projects.

nberardi commented Mar 22, 2013

@glennblock Common.Logging is a great library, I have used it on a number of projects.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

Totally blocked on it, but not at all opposed to that. I just don't want us
to reinvent the wheel just for this project!

On Fri, Mar 22, 2013 at 9:50 AM, Nick Berardi notifications@github.comwrote:

@glennblock https://github.com/glennblock Common.Logging is a great
library, I have used it on a number of projects.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15297464
.

Member

glennblock commented Mar 22, 2013

Totally blocked on it, but not at all opposed to that. I just don't want us
to reinvent the wheel just for this project!

On Fri, Mar 22, 2013 at 9:50 AM, Nick Berardi notifications@github.comwrote:

@glennblock https://github.com/glennblock Common.Logging is a great
library, I have used it on a number of projects.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15297464
.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

That didn't come out right. I was saying totally BLANKED on Common.Logging
:p

On Fri, Mar 22, 2013 at 9:57 AM, Glenn Block glenn.block@gmail.com wrote:

Totally blocked on it, but not at all opposed to that. I just don't want
us to reinvent the wheel just for this project!

On Fri, Mar 22, 2013 at 9:50 AM, Nick Berardi notifications@github.comwrote:

@glennblock https://github.com/glennblock Common.Logging is a great
library, I have used it on a number of projects.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15297464
.

Member

glennblock commented Mar 22, 2013

That didn't come out right. I was saying totally BLANKED on Common.Logging
:p

On Fri, Mar 22, 2013 at 9:57 AM, Glenn Block glenn.block@gmail.com wrote:

Totally blocked on it, but not at all opposed to that. I just don't want
us to reinvent the wheel just for this project!

On Fri, Mar 22, 2013 at 9:50 AM, Nick Berardi notifications@github.comwrote:

@glennblock https://github.com/glennblock Common.Logging is a great
library, I have used it on a number of projects.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15297464
.

@nberardi

This comment has been minimized.

Show comment
Hide comment
@nberardi

nberardi Mar 22, 2013

I agree, keep the reinvention to a minimum.

nberardi commented Mar 22, 2013

I agree, keep the reinvention to a minimum.

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Mar 22, 2013

Contributor

So far we have:

  • Using System.Diagnostics.Trace for logging.
  • Took keep things simple, as it has three levels by default (TraceInfo, TraceWarning, TraceError), so we would allow -log error, -log info, -log warning, -log verbose and -log off based on this.
  • We will create a wrapper for System.Diagnostics.Trace (e.g. TraceLogWriter) and an interface ILogWriter. Question: In which project should this be?

Did I miss anything?

Contributor

dschenkelman commented Mar 22, 2013

So far we have:

  • Using System.Diagnostics.Trace for logging.
  • Took keep things simple, as it has three levels by default (TraceInfo, TraceWarning, TraceError), so we would allow -log error, -log info, -log warning, -log verbose and -log off based on this.
  • We will create a wrapper for System.Diagnostics.Trace (e.g. TraceLogWriter) and an interface ILogWriter. Question: In which project should this be?

Did I miss anything?

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

I'd prefer we do not use trace. Using loggers is the defacto for most OSS
projects, not trace listeners. too complicated imo.

On Fri, Mar 22, 2013 at 10:23 AM, dschenkelman notifications@github.comwrote:

So far we have:

  • Using System.Diagnostics.Trace for logging.
  • Took keep things simple, as it has three levels by default
    (TraceInfo, TraceWarning, TraceError), so we would allow -log error, -log
    info, -log warning, -log verbose and -log off based on thishttp://msdn.microsoft.com/en-us/library/system.diagnostics.tracelevel.aspx
    .
  • We will create a wrapper for System.Diagnostics.Trace (e.g.
    TraceLogWriter) and an interface ILogWriter. Question: In which
    project should this be?

Did I miss anything?


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15299166
.

Member

glennblock commented Mar 22, 2013

I'd prefer we do not use trace. Using loggers is the defacto for most OSS
projects, not trace listeners. too complicated imo.

On Fri, Mar 22, 2013 at 10:23 AM, dschenkelman notifications@github.comwrote:

So far we have:

  • Using System.Diagnostics.Trace for logging.
  • Took keep things simple, as it has three levels by default
    (TraceInfo, TraceWarning, TraceError), so we would allow -log error, -log
    info, -log warning, -log verbose and -log off based on thishttp://msdn.microsoft.com/en-us/library/system.diagnostics.tracelevel.aspx
    .
  • We will create a wrapper for System.Diagnostics.Trace (e.g.
    TraceLogWriter) and an interface ILogWriter. Question: In which
    project should this be?

Did I miss anything?


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-15299166
.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 22, 2013

Member

My preference is this

  1. Use common logging as the interface. We reference that in ScriptCs.Core.
  2. ScriptCS CLI plugs in log4net via that common interface.
  3. ScriptCS.Core has no notion of log4net.
  4. We do NOT use trace listeners, but support it via the interface.
Member

glennblock commented Mar 22, 2013

My preference is this

  1. Use common logging as the interface. We reference that in ScriptCs.Core.
  2. ScriptCS CLI plugs in log4net via that common interface.
  3. ScriptCS.Core has no notion of log4net.
  4. We do NOT use trace listeners, but support it via the interface.
@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 23, 2013

Member

OK cool, fine by me. My "beef" was log4net in core :-)

Member

filipw commented Mar 23, 2013

OK cool, fine by me. My "beef" was log4net in core :-)

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 29, 2013

Member

@dschenkelman any update here on the suggestions above? There is also some feedback on usings.

Member

glennblock commented Mar 29, 2013

@dschenkelman any update here on the suggestions above? There is also some feedback on usings.

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Mar 29, 2013

Contributor

Sorry, haven't been able to spend time on this. I'll probably get this done over the weekend.

Basically this would be:

  1. Updating usings.
  2. Removing reference to log4net in all projects except for console app. Use Common.Logging instead.

Anything else?

Contributor

dschenkelman commented Mar 29, 2013

Sorry, haven't been able to spend time on this. I'll probably get this done over the weekend.

Basically this would be:

  1. Updating usings.
  2. Removing reference to log4net in all projects except for console app. Use Common.Logging instead.

Anything else?

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 29, 2013

Member

Nope. No problem on being busy, we all have that challenge :-)

I was only pinging since the PR was 11 days ago.

Thanks!

-----Original Message-----
From: "dschenkelman" notifications@github.com
Sent: ‎3/‎29/‎2013 8:02 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

Sorry, haven't been able to spend time on this. I'll probably get this done over the weekend.
Basically this would be:

  1. Updating usings.
  2. Removing reference to log4net in all projects except for console app. Use Common.Logging instead.
    Anything else?

    Reply to this email directly or view it on GitHub.
Member

glennblock commented Mar 29, 2013

Nope. No problem on being busy, we all have that challenge :-)

I was only pinging since the PR was 11 days ago.

Thanks!

-----Original Message-----
From: "dschenkelman" notifications@github.com
Sent: ‎3/‎29/‎2013 8:02 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

Sorry, haven't been able to spend time on this. I'll probably get this done over the weekend.
Basically this would be:

  1. Updating usings.
  2. Removing reference to log4net in all projects except for console app. Use Common.Logging instead.
    Anything else?

    Reply to this email directly or view it on GitHub.
@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 8, 2013

Contributor

I still need to update the program class to somehow call a method that validates the provided log level. Previously the IsValid method was available, but now it is not being used. Is it OK if I add a call to it again?

The logic I need to run is simple:

private bool IsLogLevelValid()
{
    return ValidLogLevels.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries).Contains(LogLevel);
}
Contributor

dschenkelman commented Apr 8, 2013

I still need to update the program class to somehow call a method that validates the provided log level. Previously the IsValid method was available, but now it is not being used. Is it OK if I add a call to it again?

The logic I need to run is simple:

private bool IsLogLevelValid()
{
    return ValidLogLevels.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries).Contains(LogLevel);
}
@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Apr 8, 2013

Member

I think we should leverage PowerArgs' built-in validation for this stuff. What about writing a custom ArgValidator attribute that does this? Then PowerArgs can do the validation for you 😄
We should also consider adding other relevant validation attributes, like ArgExistingFile on ScriptName...

Member

khellang commented Apr 8, 2013

I think we should leverage PowerArgs' built-in validation for this stuff. What about writing a custom ArgValidator attribute that does this? Then PowerArgs can do the validation for you 😄
We should also consider adding other relevant validation attributes, like ArgExistingFile on ScriptName...

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 8, 2013

Contributor

Great! Didn't know about custom ArgValidators. Will definetely do that.

On Mon, Apr 8, 2013 at 12:37 PM, Kristian Hellang
notifications@github.comwrote:

I think we should leverage PowerArgs' built-in validation for this stuff.
What about writing a custom ArgValidatorhttps://github.com/adamabdelhamed/PowerArgs/blob/master/PowerArgs/ArgValidatorAttributes.cs#L10attribute that does this? Then PowerArgs can do the validation for you [image:
😄]

We should also consider adding other relevant validation attributes, like
ArgExistingFilehttps://github.com/adamabdelhamed/PowerArgs/blob/master/PowerArgs/ArgValidatorAttributes.cs#L16on
ScriptName...


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-16057765
.

Contributor

dschenkelman commented Apr 8, 2013

Great! Didn't know about custom ArgValidators. Will definetely do that.

On Mon, Apr 8, 2013 at 12:37 PM, Kristian Hellang
notifications@github.comwrote:

I think we should leverage PowerArgs' built-in validation for this stuff.
What about writing a custom ArgValidatorhttps://github.com/adamabdelhamed/PowerArgs/blob/master/PowerArgs/ArgValidatorAttributes.cs#L10attribute that does this? Then PowerArgs can do the validation for you [image:
😄]

We should also consider adding other relevant validation attributes, like
ArgExistingFilehttps://github.com/adamabdelhamed/PowerArgs/blob/master/PowerArgs/ArgValidatorAttributes.cs#L16on
ScriptName...


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-16057765
.

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 10, 2013

PowerArgs also has built in support for enums. So if you make your LogLevel an enum you'll get the validation for free.

adamabdelhamed commented Apr 10, 2013

PowerArgs also has built in support for enums. So if you make your LogLevel an enum you'll get the validation for free.

dschenkelman added some commits Apr 10, 2013

Merge branch 'dev' into loglevels
Conflicts:
	src/ScriptCs.Core/packages.config
	src/ScriptCs/packages.config
@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 10, 2013

Contributor

@adamabdelhamed Great!! An enum it is.

This should be done now. Let me know if you have feedback.

Contributor

dschenkelman commented Apr 10, 2013

@adamabdelhamed Great!! An enum it is.

This should be done now. Let me know if you have feedback.

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 10, 2013

Minor thing. Case is ignored by default so your [ArgIgnoreCase] attribute doesn't need to be there. Looking back, I probably should have made an attribute called [ArgCaseSensitive], but instead I chose [ArgIgnoreCase(false)] as the way to express that you want case sensitivity.

adamabdelhamed commented Apr 10, 2013

Minor thing. Case is ignored by default so your [ArgIgnoreCase] attribute doesn't need to be there. Looking back, I probably should have made an attribute called [ArgCaseSensitive], but instead I chose [ArgIgnoreCase(false)] as the way to express that you want case sensitivity.

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 10, 2013

Contributor

I tried without the attribute and case was not ignored. I had to explicitly
add it.

For example: -log debug did not works unless I added the attribute.

On Wed, Apr 10, 2013 at 12:34 PM, Adam Abdelhamed
notifications@github.comwrote:

Minor thing. Case is ignored by default so your [ArgIgnoreCase] attribute
doesn't need to be there. Looking back, I probably should have made an
attribute called [ArgCaseSensitive], but instead I chose
[ArgIgnoreCase(false)] as the way to express that you want case
sensitivity.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-16182021
.

Contributor

dschenkelman commented Apr 10, 2013

I tried without the attribute and case was not ignored. I had to explicitly
add it.

For example: -log debug did not works unless I added the attribute.

On Wed, Apr 10, 2013 at 12:34 PM, Adam Abdelhamed
notifications@github.comwrote:

Minor thing. Case is ignored by default so your [ArgIgnoreCase] attribute
doesn't need to be there. Looking back, I probably should have made an
attribute called [ArgCaseSensitive], but instead I chose
[ArgIgnoreCase(false)] as the way to express that you want case
sensitivity.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-16182021
.

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 10, 2013

Oops. You found a bug in PowerArgs. I'm at my day job so I'll fix it later this evening. Explicitly adding that argument won't hurt in the meantime.

adamabdelhamed commented Apr 10, 2013

Oops. You found a bug in PowerArgs. I'm at my day job so I'll fix it later this evening. Explicitly adding that argument won't hurt in the meantime.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Apr 10, 2013

Member

Adam, thanks for all your help here!

On Thu, Apr 11, 2013 at 4:08 AM, Adam Abdelhamed
notifications@github.comwrote:

Oops. You found a bug in PowerArgs. I'm at my day job so I'll fix it later
this evening. Explicitly adding that argument won't hurt in the meantime.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-16184112
.

Member

glennblock commented Apr 10, 2013

Adam, thanks for all your help here!

On Thu, Apr 11, 2013 at 4:08 AM, Adam Abdelhamed
notifications@github.comwrote:

Oops. You found a bug in PowerArgs. I'm at my day job so I'll fix it later
this evening. Explicitly adding that argument won't hurt in the meantime.


Reply to this email directly or view it on GitHubhttps://github.com/scriptcs/scriptcs/pull/135#issuecomment-16184112
.

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 10, 2013

No problem. This is fun, and if you guys find a bug or two it's goodness for both projects (free testing for me).

adamabdelhamed commented Apr 10, 2013

No problem. This is fun, and if you guys find a bug or two it's goodness for both projects (free testing for me).

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 12, 2013

FYI - The latest version(1.5.0.0) has this fix if you want to pick it up and remove that unneeded attribute.

adamabdelhamed commented Apr 12, 2013

FYI - The latest version(1.5.0.0) has this fix if you want to pick it up and remove that unneeded attribute.

@den-mentiei den-mentiei referenced this pull request Apr 12, 2013

Closed

PowerArgs update #201

@den-mentiei

This comment has been minimized.

Show comment
Hide comment
@den-mentiei

den-mentiei Apr 12, 2013

Contributor

Just created another one issue about PowerArgs update and fixed it in #202. :)

Contributor

den-mentiei commented Apr 12, 2013

Just created another one issue about PowerArgs update and fixed it in #202. :)

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 14, 2013

Contributor

Is there anything else to be done for this to get merged (besides merging with the latest commit in dev)? Just asking to see if anything else is currently expected on my side.

Contributor

dschenkelman commented Apr 14, 2013

Is there anything else to be done for this to get merged (besides merging with the latest commit in dev)? Just asking to see if anything else is currently expected on my side.

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 17, 2013

Contributor

@glennblock @filipw @jrusbatch any update on this?

Contributor

dschenkelman commented Apr 17, 2013

@glennblock @filipw @jrusbatch any update on this?

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Apr 20, 2013

Member

Hi @dschenkelman

I have been travelling / on vacation. Sorry for the delay. I will look at it this weekend now that I am back.

-----Original Message-----
From: "dschenkelman" notifications@github.com
Sent: ‎4/‎17/‎2013 7:19 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

@glennblock @filipw @jrusbatch any update on this?

Reply to this email directly or view it on GitHub.

Member

glennblock commented Apr 20, 2013

Hi @dschenkelman

I have been travelling / on vacation. Sorry for the delay. I will look at it this weekend now that I am back.

-----Original Message-----
From: "dschenkelman" notifications@github.com
Sent: ‎4/‎17/‎2013 7:19 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

@glennblock @filipw @jrusbatch any update on this?

Reply to this email directly or view it on GitHub.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Apr 27, 2013

Member

@dschenkelman it looks like this is not ready to merge. Can you pull the latest from dev?

Member

glennblock commented Apr 27, 2013

@dschenkelman it looks like this is not ready to merge. Can you pull the latest from dev?

Merge branch 'dev' into loglevels
Conflicts:
	src/ScriptCs/Command/InstallCommand.cs
	src/ScriptCs/CompositionRoot.cs
	src/ScriptCs/Program.cs
	src/ScriptCs/packages.config
	test/ScriptCs.Tests/CommandFactoryTests.cs
@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman
Contributor

dschenkelman commented Apr 27, 2013

@glennblock done!

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Apr 27, 2013

Member

@dschenkelman great, looking it over now!

Member

glennblock commented Apr 27, 2013

@dschenkelman great, looking it over now!

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Apr 27, 2013

Member

Ok @dschenkelman, review looks good! One thing I noticed though is you display [Thread] for info.

This feels like debug level a little confusing. Can you change the format if is info to not show the thread?

-----Original Message-----
From: "dschenkelman" notifications@github.com
Sent: ‎4/‎27/‎2013 5:49 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

@glennblock done!

Reply to this email directly or view it on GitHub.

Member

glennblock commented Apr 27, 2013

Ok @dschenkelman, review looks good! One thing I noticed though is you display [Thread] for info.

This feels like debug level a little confusing. Can you change the format if is info to not show the thread?

-----Original Message-----
From: "dschenkelman" notifications@github.com
Sent: ‎4/‎27/‎2013 5:49 AM
To: "scriptcs/scriptcs" scriptcs@noreply.github.com
Cc: "Glenn Block" glenn.block@gmail.com
Subject: Re: [scriptcs] Added support for -log parameter using log4net.Writing additional messages to log. (#135)

@glennblock done!

Reply to this email directly or view it on GitHub.

@dschenkelman

This comment has been minimized.

Show comment
Hide comment
@dschenkelman

dschenkelman Apr 29, 2013

Contributor

@glennblock updated the code to work like this:

  • Display thread when log level is either debug or trace.
  • Not display thread when log level is either info or error.
Contributor

dschenkelman commented Apr 29, 2013

@glennblock updated the code to work like this:

  • Display thread when log level is either debug or trace.
  • Not display thread when log level is either info or error.
@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Apr 29, 2013

Member

Looks great. I think we might want to tweak some of the actual log messages BUT we can do that later.

Member

glennblock commented Apr 29, 2013

Looks great. I think we might want to tweak some of the actual log messages BUT we can do that later.

glennblock added a commit that referenced this pull request Apr 29, 2013

Merge pull request #135 from dschenkelman/loglevels
Added support for -log parameter using log4net. Writing additional messages to log.

@glennblock glennblock merged commit 3914ca0 into scriptcs:dev Apr 29, 2013

1 check passed

default TeamCity Build ScriptCS :: CI Build (dev) finished: Tests passed: 72
Details

ztone pushed a commit to ztone/scriptcs that referenced this pull request May 4, 2014

Merge pull request #135 from dschenkelman/loglevels
Added support for -log parameter using log4net. Writing additional messages to log.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment