Review of Jose Armando Garcia Sancio's std.log

[Jose Armando Garcia]

On Mon, Feb 13, 2012 at 2:49 PM, David Nadlinger <see at klickverbot.at> wrote:
> On 2/13/12 4:50 PM, David Nadlinger wrote:
>>
>>
>> https://github.com/jsancio/phobos/commit/d114420e0791c704f6899d81a0293cbd3cc8e6f5
>>
>> Docs: http://jsancio.github.com/phobos/phobos/std_log.html
>
>
> A few small points from a first pass through the docs:
>
> Spelling nits:
>  - potion -> position
>  - enviroment -> environment
>  - arguements -> arguments
>  - explictly -> explicitly
>  - fileter -> filter
>  - persiste -> persist
>  - genereated -> generated
>  - brakets -> brackets
>  - paramater -> parameter
>  - compuration -> computation
>
Thanks. I cleaned this up. I'll pass the rest through a spellchecker.

> The introductory section needs some copy editing, e.g.:
>  - In the first paragraph, every sentence starts with »The module«
>  - disabled/enabled -> disable/enable
>  - »In the other« -> »in the order«
>

I fix it some what. Still not a big fan of the current introductory
section. My current thinking for introductory section is to write
something small that every developer should read. Probably something
example driven. If they need details into how to configure the module
then they can read the rest of the document. Thoughts?

> You define the Severity enum members starting with fatal as 0. Why not the
> other way round – so that severityA < severityB would do what you (or at
> least I) would expect?
>

Internally I wanted to use the same representation for severity as I
would use for verbosity. I wanted 0 to represent the highest
importance and increasing numbers to have lower severity/importance.
This would allow users freely pick lower and lower verbose message as
they develop their application without having to worry how the
application is configured in the deployment/production environment.

> Include a cross-reference to log() in the Severity comment?
>
Done

> Maybe clarify the meaning of »can be logged« (i.e. the severity level is not
> completely disabled) in the LogFilter docs?
>
Thanks. This part of the document was some what confusing. It should
be a little better now.

> Are the explicit to!string calls in the first LogFilter example required?
>
> config.logger: First line is missing a full stop, »The default value a
> FileLogger.«, »change and configure« (where is the difference?)
>
No, it is not. Fixed.

> »Create a configuration object based on the passed parameter.« is slightly
> misleading, because parseCommandLine() doesn't actually create an object.
>

Good catch. Fixed. I have been thinking of changing the signature of
the Configuration classes to look more like a builder by using the
"with..." pattern in the method signature. Thoughts?
>
> I'll post a more in-depth review later.
>
> David

Thanks a lot. Your comments were helpful.