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

[Vlad Skvortsov]

Hi!

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).

I also don't feel too comfortable with some of the design decisions 
here. My comments follow.


[skipped]

> Modified: src/trunk/DITrack/Command/list.py
> ===================================================================
> --- src/trunk/DITrack/Command/list.py	2007-03-22 09:02:50 UTC (rev 1485)
> +++ src/trunk/DITrack/Command/list.py	2007-03-23 15:07:58 UTC (rev 1486)
> @@ -51,7 +51,7 @@
>  
>          for e in opts.fixed[1:]:
>              try:
> -                f = DITrack.DB.Filter(e)
> +                f = DITrack.DB.Filter(e, globals)
>   

'globals' is way too dt (command line client) specific thing to make use 
of it in a database module which may be reused by different kinds of 
clients. I suggest passing a dictionary instead, e.g.:

vars = dict(os.environ)
vars["DT:USER"] = fetch_user_name_from_wherever()

f = DITrack.DB.Filter(e, vars)

[skipped]

> Modified: src/trunk/DITrack/Command/new.py
> ===================================================================
> --- src/trunk/DITrack/Command/new.py	2007-03-22 09:02:50 UTC (rev 1485)
> +++ src/trunk/DITrack/Command/new.py	2007-03-23 15:07:58 UTC (rev 1486)
> @@ -45,7 +45,7 @@
>  
>          db = DITrack.Util.common.open_db(globals, opts)
>  
> -        globals.get_username(opts, db)
> +        globals.get_username(opts, db.cfg.users)
>   

Hm, again - db.cfg.users is way too implementation-dependent to hardcode 
it in every single place where we need to figure out the current user 
name. I'd strongly prefer passing database object instead (thus if we 
need to change it, we don't have to chase for it throughout all the code).

[skipped]

> Modified: src/trunk/DITrack/Configuration.py
> ===================================================================
> --- src/trunk/DITrack/Configuration.py	2007-03-22 09:02:50 UTC (rev 1485)
> +++ src/trunk/DITrack/Configuration.py	2007-03-23 15:07:58 UTC (rev 1486)
> @@ -257,7 +257,7 @@
>      def __getitem__(self, key):
>          return self.items[key]
>  
> -    def __init__(self, path):
> +    def __init__(self, path, globals=None):
>   

Same thing - 'globals' doesn't seem to fit well here.

[skipped]

> @@ -353,6 +353,6 @@
>              self.versions)
>  
>          try:
> -            self.filters = FilterCfg(self.path["filters"])
> +            self.filters = FilterCfg(self.path["filters"], globals)
>   

Ditto.

> @@ -183,7 +161,7 @@
>      Class representing complete database object.
>      """
>  
> -    def __init__(self, path, svn_path):
> +    def __init__(self, path, globals):
>          """
>          Opens a DITrack database at PATH. The SVN path is a path to the 
>          Subversion command line client executable.
>   

Same here - database object should not depend on client-specific global 
variable storage.

[skipped]

> Modified: src/trunk/DITrack/dt/globals.py
> ===================================================================
> --- src/trunk/DITrack/dt/globals.py	2007-03-22 09:02:50 UTC (rev 1485)
> +++ src/trunk/DITrack/dt/globals.py	2007-03-23 15:07:58 UTC (rev 1486)
> @@ -119,7 +119,7 @@
>              if not (st[stat.ST_MODE] & stat.S_IEXEC):
>                  DITrack.Util.common.err(msg + "is not executable")
>  
> -    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.

Keep going :-)

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