On 12/19/12, Peter Koželj <[email protected]> wrote:
>>
>> If my comment above is accurate : no need to do nothing of this . This
>> is happening every day , every where in Trac
>>
>> Maybe I didn't understand the initial statement about option defaults .
>> CMIIW
>>
>
> We were not on the same page here :( as I misunderstood your initial
> observation.
> I was considering env.config.options to automatically enumerate labels
all labels at once ? well if that's the case you just cannot do that .
I was thinking of options one-by-one
> but
> then I had nowhere to put the defaults.
now it seems I understand .
> Anyway this has nothing to do with your observation and I apologize for
> confusion.
>
nevermind :)
> I can use trac.config.Option instead of self.env.config.get if this is the
> preferred way.
> So is trac.config.Option the preferred way? Is if so is that just a
> convention or are there deeper technical reasons to prefer one over
> another?
>
beyond options docs and defaults , the major benefit I can see is that
these become instance methods too => more readable code .
>
>> Please review the implementation of trac.web.chrome.Chrome.dispatch
>> method and request callbacks . Using data may lead to clashes with
>> other active request handlers
>>
>
> Reviewed, it was an useful exercise :)
... and painful ... I know . the issue fixed in r
> My conclusions:
> - data needs to be dict, but of corse that does not mean it is an
> appropriate holder for the labels by itself
... and in post_process_request it may be None too ;)
> - req.chrome is always evaluated by Chrome.render_template, but I agree
> that the same should be avoided by request filter methods
>
yes , but sometimes no template is rendered at all while processing a
request e.g. RPC requests do not use chrome at ... why bother
instantiating it in post process request ?
;)
> So if we don't want to add it to data or force lazy evaluation of
> req.chrome, the only thing that remains is a new property req.labels as:
>
> {{{
>
> def post_process_request(self, req, template, data, content_type):
> ...
> req.callbacks['labels'] = lambda req: self._get_whitelabelling()
> ...
>
> }}}
>
> This works but doing req.labels['whatever'] does leave a bit strange taste
> in my mouth as it seems out of place.
>
Yes , I confess the same happens to me . If req.chrome wasn't such a
problematic feature it would be the right place to include 'labels' .
Question : Instead of post-processing request by injecting white
labeling data somewhere is it possible to change this at stream
filters level instead ?
If so neither chrome nor data (and related complications) would be
needed . Instance methods would be more than enough . What do you
think ?
--
Regards,
Olemis.
Blog ES: http://simelo-es.blogspot.com/
Blog EN: http://simelo-en.blogspot.com/
Featured article:
