[ditrack commit] r1486 - in src/trunk: . DITrack DITrack/Command DITrack/Util DITrack/dt

[Vlad Skvortsov]

Ivan Glushkov wrote:
>> I'm sure this commit does what it needed, but I have a few concerns
>> about it. First, as I've said earlier, I'm not in favor of idea of
>> mixing environment variables with DITrack-specific variables. Thus my
>> suggestion is to have clear distinction (by means of syntax) between 
>> them:
>>
>> $VAR == environment variable
>> $DT:VAR == DITrack-specific variable (that *may* happen to have the same
>> value as an environment variable).
> What do you mean 'mixing environment variables with DITrack-specific
> variables'.  They are mixed if dt-variables don't begin with "DT:"?

Yes. By looking at a variable name a user should be able to infer where 
the data comes from.


[skipped]

>> > -    def get_username(self, opts, db):
>> > +    def get_username(self, opts, correct_users=None):
>> >
>>
>> Why not something like this instead:
>>
>> def get_username(self, opts, db, valid_users_only=True)
>>
>> ?
>>
>> It's 'globals' that should depend on 'Database', not the other way 
>> around.
>
> To correctly form filters while creating Database() we should know
> username (as filters refer to the ditrack username).
> So, we should determine it with 'globals.get_username()' before
> 'open_db()'(or 'Database()') and to pass it there in 'globals' object.
> username belongs only to 'globals' object, doesn't it?
> So, database depends on 'globals' and not the reverse

I meant that 'Database' has much broader context than 'globals'. 
Database could be used by arbitrary number of clients, which should not 
be obliged to instantiate 'globals' object similar to that in 'dt'.

>
> Frankly speaking, we don't need database object at all to determine
> what the username is in the environment, do we?

Correct. Though we need the Database to determine if the username is valid.

> We only should know about 'os.environ' and 'opts.var'. So why should
> we pass db to 'get_username()'?

Historically we did this to check the validity of the username right away.

>
> So i changed 'get_username()' as it wanted 'db' as parameter. So we 
> couldn't do:
>
> globals.get_username(opts,db)
> db = open_db(globals,opt) 

Hmm. I guess what we need to do is:

1) make get_username() do just one thing -- figuring out the username;
2) pass raw username to the Database constructor;
3) pass open mode (readonly/modify) to the Database constructor.

In the readonly mode, Database() shouldn't check for the username 
validity. Otherwise it should and will raise an exception if the name 
passed is not valid.

I think it's along the lines with something you've proposed in one of 
the emails. Comments?

-- 
Vlad Skvortsov, vss at 73rus.com, http://vss.73rus.com