[webgen-devel] [TL] Better patch

[Thomas Leitner]

From: Thomas Leitner <t_leitner at gmx.at>
Date: 29. August 2007 21:54:39 GMT+02:00
To: Andrea Ferro
Subject: Re: Better patch

>> Thanks for the patch! I have looked over it and the new logging  
>> facility seems really cool. Some comments:
>>
>> * If I set Core/WebSite:tracingLevel to 0 in the configuration  
>> file and don't use the loggingLevel switch from the CLI program, I  
>> get no trace statements - should this be so? I think the problem  
>> stems from the GlobalLogger.logger since its logging level is set  
>> to ERROR and therefore no statements are printed. I don't know if  
>> this should be so by design, however, if I set the tracingLevel  
>> explicitly in the configuration file I do want the output,  
>> independent from the global logging level.
>
> No. You do not want to see it in all cases.
> You may think you are because sometimes the naming is confusing and  
> the filtering logic seems reversed.
>
> Recall:
> loggingLevel is really a log filtering level. Defaults filter for  
> ERROR in the upstream logger and DEBUG locally. This means all  
> events are passed up (local default filters at DEBUG) and filtered  
> upstream at ERROR (you'll see ERROR and FATAL, you do not see WARN,  
> INFO and DEBUG).
>
> tracingLevel is the severity level for the trace events. It  
> currently defaults to DEBUG. So traces are DEBUG severity per default.
>
> Your setting Core/WebSite:tracingLevel to 0 is ineffective because:  
> it's filtered upstream AND it's actually the default setting.
>
> Try setting Core/WebSite:tracingLevel to 3 or 4 and you'll see the  
> WebSite traces (but not the DEBUG logs or traces from other objects.

Okay, I understand know. One small problem remains: when setting Core/ 
PluginManager:tracingLevel to 3 all tracing statements have the  
severity level ERROR, if setting this parameter to 2, all tracing  
statements have severity level WARN. I think the severity level  
should always be the same for the tracing statements, ideally  
something like TRACE.

> In other words:
> * normal logging:
> 1) in normal logging calls the +severity+ is explicit in the call  
> itself (even with #log where it's generally a constant parameter)
> 2) normal logging calls result in only a value for the final  
> +message+ OR in *both* a value for the final +message+ and a value  
> for the final +progname+ (only using #log and explicitly passing  
> nil you can specify a final +progname+ but no value for the final  
> +message+)
> 3) if a normal logging call results in only a value for the final  
> +message+ then a default value stored in the top ancestor logger is  
> used for +program+
>
> * tracing
> 1) in tracing +severity+ is runtime configurable (the tracingLevel  
> value)
> 2) in tracing both a value for the final +message+ AND a value for  
> the final +progname+ will be passed up the chain. The value for  
> +message+ is the same as it would be in normal log events, the  
> value for +progname+ is the one you specified plus data from the  
> stack (if you specified nothing it's "TRACE" plus the data from the  
> stack).
> 3) of course this means that if a normal non-hacked ::Logger is  
> upstream, the default for +progname+ is never used for traces
>
> Of course setting a value of +tracingLevel+ does not affect the  
> normal +severity+ filtering.
> The defaults currently are:
> - ERROR(=3) for filtering at the top ancestor (overridable from the  
> command line with -V)
> - DEBUG(=0) for filtering at intermediate DelegatingLoggers (that  
> is do not filter)
> - DEBUG(=0) for default tracingLevel
>
> This means all trace events are generated at DEBUG severity and  
> filtered at the top ancestor logger.
>
> To see ALL events set the top filtering to DEBUG ( CLI -V 0 ) and  
> leave the rest at default.

Thanks for the overview!

>> * Webgen::WebSite.new( 'dir' ).reset returns the website object. I  
>> know it is more concise to only need a single line but does one  
>> really expect to get the website object back from the reset method?
>
> Well, what exactly would one really expect reset to return?
> Nothing?
> An error state?
>
> WebSite#reset cannot really fail (if it does it's an exception sort  
> of failure). And it has no really meaningful return value. So  
> either it returns the WebSite or the PluginManager. But if you want  
> the PluginManager you can do ...reset.plugin_manager... so the only  
> reasonable thing to return is self.
>
> I'm doing the same for other methods too now (maybe not yet in the  
> patch I sent you). Methods that do not have a meaningful return  
> value now return self so that they are chain-able. Also methods  
> that only have a boolean return value 'could' return self or nil  
> (but this is less useful as they would not be chainable in general.  
> But if there's a way to ensure that if some given preconditions are  
> met the method returns true, than a user that does know those  
> preconditions are always met at a given point in the code sequence  
> can still chain up calls).
> If you know C/C++ the idea is: global functions may be void.  
> Methods that would normally be void may actually be turned into  
> returning a reference to the object and return *this. It does no  
> harm. It does not really slow up code. And may be usefull to callers.
>
> This is an extension to chained assignment (the logic by which an  
> assignment expression is an expression and not a statement as it  
> used to be in past languages).
>
> Besides: even if requiring #reset as an explicit call is functional  
> to the modularity of the system, I still expect many users to do  
> WebSite.new.reset or even WebSite.new.reset.render (and in fact  
> render now also returns self so you can do it all and still save  
> the instanced objects in a variable!!).

Chaining is a fine thing, that's true. But we need to be careful in  
selecting the methods which return self! And it should be stated in  
the RDOC documentation for the method.

> I have some questions for you. Some not webgen related. I may  
> probably find the answers myself, but if you know them it may be  
> faster for you to answer (or point me to documentation) then for me  
> to google around and browse sources.
>
> - I'm not updating Rakefile and the list of files while I add them.  
> I figure we do not need to "package" for the moment and can do that  
> at a later time. Is that ok for you? (also I'm not very sure of  
> what should be changed!)

I will update the packaging code in the Rakefile to use the Rubyforge  
library or hoe to automate releasing webgen. Still need to look  
around a bit to see if there are other complete solutions like hoe  
around. So just leave the Rakefile as it is, I'll see to it.

> - rDoc seems to highlight all words that have some meaning in the  
> project. When it sees <Core/Configuration> in a comment it  
> hyperlinks Core (and not Configuration as that's not a class in  
> webgen yet). Is there a way to tell it not to?

I don't really know. I can imagine that there is some markup that  
prevents RDOC from automatically hyperlinking a word but I never  
searched nor used that feature, if it exists. It's not nice that Core  
gets hyperlinked, but I can live with it.

> - rDoc have a flag to show the (normally hidden) leading # before  
> the name of a method in a comment. But it seems to always hyperlink  
> all words that are method names regardless. So what's the reason to  
> have a leading # (I could not find ANY docs for rDoc that say  
> something about the leading # and any syntax for hyperlinking  
> except the reference to the leading # in the descriptions of CLI  
> options)

Never used that option. And since the problem with the 'highlight-all- 
method-names' approach of RDOC I don't see any use for this option.

> - rDoc seems to pick the 'main' comment for a top level object  
> (like module Webgen) from the last file it happens to process that  
> declares it and have a comment. Is there a way to control it?

Don't know, I need to look this up myself.

> - what exactly is the logic of having two possible places for the  
> data directory (see #data_dir in config.rb)? Why are they checked  
> in that order?

There are basically three ways how webgen can be used:
1. via Rubygems
2. by installing it via setup.rb
3. not installing, just running from the package directory like we  
currently do

If webgen gets installed via Rubygems, the whole package is extracted  
under .../gems/1.8/gems/webgen-0.5.0 (or whatever) and therefore has  
the same directory structure as in approach 3. This handled by the  
first line in Webgen.data_dir. If such a directory is not found (and  
this is normally only when webgen has been installed via setup.rb  
since then everything below the data directory is located under /usr/ 
share/), the datadir configuration variable of Ruby is used to  
determine the valid data directory for webgen.

> - Why system plugin bundles in data/webgen/plugins/*/*.plugin  
> instead of lib/webgen/plugins/*/*.plugin ? Or maybe even diretcly  
> lib/webgen/*/*.plugin (without the 'plugin' superdirecory)?

I don't have a real notion of 'system plugin bundles'. It is just  
that some plugins are more used than others, like for instance the  
Core/FileHandler plugin which has the main render algorithm. However,  
sometimes also the Core/FileHandler is not needed, for example, when  
the CLI is invoked with  the Cli/Commands/About plugin. So the plugin  
system of webgen is, more or less, independent from how it is used in  
webgen. And I don't know why some plugins should get a special  
treatment only because they are used more often. In the webgen 0.4.x  
series the Core/Configuration and some other plugins had a special  
status and I tried to avoid this in webgen 0.5.0.

And the directories directly under data/webgen/plugins are just used  
to separate the plugins. We could also put all *.plugin directories  
directly under data/webgen/plugins - that would also be fine by me.

> - Having Core/Configuration as a plugin (especially having it  
> loaded by PluginManager when it *is* global config data that could  
> be usefull before it's loaded) is counter intuitive. And maybe the  
> same is true for cachemanager and filehandler. I already asked it  
> above in this mail: why not reserving Core/* namespace and make  
> those special/pseudo plugins like WebGen and PluginManager (and  
> even FileConfigurator for that sake) all "preloaded" and embedded  
> in the system? After all that's the common meaning for "core". If  
> any one of those in Core/* does not fit an embedded+preexisting  
> logic for some reason it can go somewhere else (Support/* for  
> example). Alternatively we can make another prefix for WebSite,  
> Configuration, PluginManager etc but I cannot think of any better  
> name than Core (maybe Webgen/* could do, but I do not really feel  
> good about it).

Hmm... I get your point. However, Core/Configuration just provides  
parameter values which are available without loading a plugin, only  
the plugin infos need to be loaded. And the Core/CacheManager is not  
always needed (e.g. when the CLI is invoked with anything other than  
the run command). Same holds true for Core/FileHandler. And when the  
other needed the get autoloaded by PluginManager#[], so no harm done.  
I also don't want this unless there is a good use case where this  
embedded solution is needed.

Btw. I like the spelling of webgen to be just all lowercase ;-)

-- Thomas

ps. Regarding IRC: it works for me from different locations and  
computers. The channel is #webgen on irc.freenode.net

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://rubyforge.org/pipermail/webgen-devel/attachments/20070920/f483ffe2/attachment-0001.html