incubator-bloodhound-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Olemis Lang <ole...@gmail.com>
Subject Re: [Whitelabeling] Quick assessment of (current) whitelabelling implementation .
Date Wed, 19 Dec 2012 17:04:29 GMT
On 12/19/12, Peter Koželj <peter@digiverse.si> 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:

Mime
View raw message