Hi,
while I went through my patch queues today to push as many patches upstream as
possible, I spotted some stuff that is not really a bug but I don't like it as
coding style:
1. Importing a package/module instead of classes
-import trac.perm as perm
+from trac.perm import PermissionCache, PermissionSystem
2. Repeated use of string constants when you can easily use a local variable
(see ask_cleanup_postgres_dbparams for context).
Should I file these as tickets or do you consider these things as perfectly
reasonable so it should left as it is now?
fs
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Trac
Development" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/trac-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
diff -r 85e621668d22 trac/db/postgres_backend.py
--- a/trac/db/postgres_backend.py Sun Aug 30 15:55:16 2009 +0200
+++ b/trac/db/postgres_backend.py Sun Aug 30 15:55:47 2009 +0200
@@ -113,20 +113,21 @@
scheme, db_prop = _parse_db_str(db_url)
db_prop.setdefault('params', {})
db_name = os.path.basename(db_prop['path'])
+ db_params = db_prop['params']
args = [self.pg_dump_path, '-C', '-d', '-x', '-Z', '8']
if 'user' in db_prop:
args.extend(['-U', db_prop['user']])
- if 'host' in db_prop['params']:
- host = db_prop['params']['host']
+ if 'host' in db_params:
+ host = db_params['host']
else:
host = db_prop.get('host', 'localhost')
args.extend(['-h', host])
if '/' not in host:
args.extend(['-p', str(db_prop.get('port', '5432'))])
- if 'schema' in db_prop['params']:
- args.extend(['-n', '"%s"' % db_prop['params']['schema']])
+ if 'schema' in db_params:
+ args.extend(['-n', '"%s"' % db_params['schema']])
dest_file += ".gz"
args.extend(['-f', dest_file, db_name])
diff -r 0f018f4fd496 trac/ticket/tests/api.py
--- a/trac/ticket/tests/api.py Sun Aug 30 15:48:32 2009 +0200
+++ b/trac/ticket/tests/api.py Sun Aug 30 15:55:15 2009 +0200
@@ -1,4 +1,4 @@
-import trac.perm as perm
+from trac.perm import PermissionCache, PermissionSystem
from trac.ticket.api import TicketSystem
from trac.ticket.model import Ticket
from trac.test import EnvironmentStub, Mock
@@ -10,7 +10,7 @@
def setUp(self):
self.env = EnvironmentStub()
- self.perm = perm.PermissionSystem(self.env)
+ self.perm = PermissionSystem(self.env)
self.ticket_system = TicketSystem(self.env)
self.req = Mock()
@@ -81,7 +81,7 @@
ts = TicketSystem(self.env)
self.perm.grant_permission('anonymous', 'TICKET_CREATE')
self.perm.grant_permission('anonymous', 'TICKET_MODIFY')
- self.req.perm = perm.PermissionCache(self.env)
+ self.req.perm = PermissionCache(self.env)
get_actions = lambda status: self._ts_get_available_actions(ts, status)
self.assertEqual(['leave', 'resolve', 'reassign', 'accept'],
get_actions({'status': 'new'}))
@@ -96,7 +96,7 @@
def test_available_actions_no_perms(self):
ts = TicketSystem(self.env)
- self.req.perm = perm.PermissionCache(self.env)
+ self.req.perm = PermissionCache(self.env)
get_actions = lambda status: self._ts_get_available_actions(ts, status)
self.assertEqual(['leave'], get_actions({'status': 'new'}))
self.assertEqual(['leave'], get_actions({'status': 'assigned'}))
@@ -107,7 +107,7 @@
def test_available_actions_create_only(self):
ts = TicketSystem(self.env)
self.perm.grant_permission('anonymous', 'TICKET_CREATE')
- self.req.perm = perm.PermissionCache(self.env)
+ self.req.perm = PermissionCache(self.env)
get_actions = lambda status: self._ts_get_available_actions(ts, status)
self.assertEqual(['leave'], get_actions({'status': 'new'}))
self.assertEqual(['leave'], get_actions({'status': 'assigned'}))
@@ -119,7 +119,7 @@
# CHGPROP is not enough for changing a ticket's state (#3289)
ts = TicketSystem(self.env)
self.perm.grant_permission('anonymous', 'TICKET_CHGPROP')
- self.req.perm = perm.PermissionCache(self.env)
+ self.req.perm = PermissionCache(self.env)
get_actions = lambda status: self._ts_get_available_actions(ts, status)
self.assertEqual(['leave'], get_actions({'status': 'new'}))
self.assertEqual(['leave'], get_actions({'status': 'assigned'}))
