Refactor trac.env.Environment to use @lazy decorator on 3 properties

[Ryan J Ollos]

I've been studying some code in trac.env.Environment and if I'm understanding things correctly, it looks like we could refactor some code that hasn't been touched in a while, and make additional use of the @lazy decorator.

• The property decorator was added to _component_rules when it was added in [9035].
• The property decorator was added to href and abs_href in [10639]
• The lazy decorator was added in [10737]

Therefore from the history, it looks like the code could just be due for some refactoring now that lazy is available.

I'll submit a patch, but I'll leave it for a while until I get some feedback, since the only major advantage to making these changes is code simplification, and I'd like to have a few code reviews before pushing the changes. The changes are against 1.0-stable, but it might make more sense to push the changes to the trunk.

[Ryan J Ollos]

Please see changes in repos:rjollos.git:t11245.

[Jun Omae]

At least, I have two points.

[a182ff98/rjollos.git]

Environment._abs_href is still used in _dispatch_request at source:rjollos.git/trac/web/main.py@t11245#L490​.

[c30ea58f/rjollos.git]

If we use @lazy for _component_rules method, I don't think that we should use self._rules attribute.

[Peter Suter]

Some minor points:

[8626c4e8/rjollos.git]

(Just for the record: SplitResult.path is available since Python 2.5.)

[13062050/rjollos.git]

Elsewhere in Trac tempfile.mkdtemp(prefix='trac-...') is used to get a prefix instead of a suffix. Also, os.path.join(tempfile.gettempdir(), ...) is used in several other places. Do you plan to replace them too?

[Ryan J Ollos]

Replying to jomae:

[a182ff98/rjollos.git]

Environment._abs_href is still used in _dispatch_request at source:rjollos.git/trac/web/main.py@t11245#L490​.

Thanks, apparently I didn't do enough grepping in the codebase.

Based on testing I've done it seems to be okay to replace env._abs_href with env.abs_href. It looks like the method decorated with lazy executes once and then sets the instance attribute, so I can't see any problem with setting this attribute again later in the code.

Further grepping in the codebase reveals a test case​ that also needs a minor update.

[c30ea58f/rjollos.git]

If we use @lazy for _component_rules method, I don't think that we should use self._rules attribute.

That was slopping editing on my part. I'll replace it with _rules.

I've made both of those changes on my branch.

[Ryan J Ollos]

Replying to psuter:

[13062050/rjollos.git]

Elsewhere in Trac tempfile.mkdtemp(prefix='trac-...') is used to get a prefix instead of a suffix. Also, os.path.join(tempfile.gettempdir(), ...) is used in several other places. Do you plan to replace them too?

Thanks, I will use the prefix keyword argument to mkdtemp and perform replacements with mkdtemp throughout the codebase.

It looks like lazy could be utilized in quite a few more classes as well, but I thought I'd see how these changes went and then possibly tackle similar refactorings in another ticket.

[Ryan J Ollos]

I forgot to mention something import that motivated this change.

>>> print os.path.join(tempfile.gettempdir(), 'trac-tempenv')
/tmp/trac-tempenv
>>> print tempfile.mkdtemp(prefix='trac-tempenv-')
/tmp/trac-tempenv-GGSeJa

If a previous test run aborted leaving the temp directory present, then subsequent test runs will fail with a message such as:

======================================================================
ERROR: test_abs_href (__main__.EnvironmentTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/tests/env.py", line 60, in setUp
    self.env = Environment(env_path, create=True)
  File "/home/user/Workspace/t11245/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11245/teo-rjollos.git/trac/env.py", line 280, in __init__
    self.create(options)
  File "/home/user/Workspace/t11245/teo-rjollos.git/trac/env.py", line 556, in create
    os.mkdir(self.get_log_dir())
OSError: [Errno 17] File exists: '/tmp/trac-tempenv/log'

By using tempfile.mkdtemp, I think we'll make the test execution more robust. This could be particularly useful with automated test runs on a server, so that the caller of setup.py test doesn't have to do any external cleanup. The only reason I could see not to make this change is if the environment directory name trac-tempfile was important (as opposed to the unique trac-tempenv-GGSeJa), but I can't see a reason why that would be the case.

[Ryan J Ollos]

Has anyone encountered this error with the two reST functional test cases (ReStructuredTextWikiTest and ReStructuredTextCodeBlockTest​)?

2013-07-17 17:30:05,613 Trac[api] WARNING: HTML preview using ReStructuredTextRenderer failed:
Traceback (most recent call last):
  File "/home/user/Workspace/t11245-2.7/teo-rjollos2.git/trac/mimeview/api.py", line 773, in render
    rendered_content, filename, url)
  File "/home/user/Workspace/t11245-2.7/teo-rjollos2.git/trac/mimeview/rst.py", line 278, in render
    'warning_stream': False})
  File "/home/user/Workspace/t11245-2.7/local/lib/python2.7/site-packages/docutils-0.10-py2.7.egg/docutils/core.py", line 448, in publish_parts
    enable_exit_status=enable_exit_status)
  File "/home/user/Workspace/t11245-2.7/local/lib/python2.7/site-packages/docutils-0.10-py2.7.egg/docutils/core.py", line 657, in publish_programmatically
    pub.set_components(reader_name, parser_name, writer_name)
  File "/home/user/Workspace/t11245-2.7/local/lib/python2.7/site-packages/docutils-0.10-py2.7.egg/docutils/core.py", line 93, in set_components
    self.set_reader(reader_name, self.parser, parser_name)
  File "/home/user/Workspace/t11245-2.7/local/lib/python2.7/site-packages/docutils-0.10-py2.7.egg/docutils/core.py", line 82, in set_reader
    reader_class = readers.get_reader_class(reader_name)
  File "/home/user/Workspace/t11245-2.7/local/lib/python2.7/site-packages/docutils-0.10-py2.7.egg/docutils/readers/__init__.py", line 113, in get_reader_class
    return module.Reader
AttributeError: 'module' object has no attribute 'Reader'

The markup used in the test case renders normally in this environment when I run tracd, but functional test cases repeatedly fail.

[Ryan J Ollos]

Replying to rjollos:

The markup used in the test case renders normally in this environment when I run tracd, but functional test cases repeatedly fail.

I was able to reproduce the issue while running TracStandalone, and proposed a patch in #11248.

[Ryan J Ollos]

I've rebased the changes in repos:rjollos.git:t11245.2, including changes for all of the suggestions from this ticket.

[Ryan J Ollos]

Committed to 1.0-stable in [11863:11870]. [11863:11867] merged to trunk in [11871] and [11868:11870] merged to trunk in [11872].

[Ryan J Ollos]

While replacing/adding the lazy decorator on the 3 methods I was thinking that I should add a test that the method really is "lazy", but I never got around to exploring that. I saw something similar to the following while doing some reading recently, so I propose these additional tests:

trac/tests/env.py

@@ -65 +65 @@
         self.env.shutdown() # really closes the db connections
         shutil.rmtree(self.env.path)
 
+    def test_db_exc(self):
+        db_exc = self.env.db_exc
+        self.assertTrue(hasattr(db_exc, 'IntegrityError'))
+        self.assertTrue(db_exc is self.env.db_exc)
+
     def test_abs_href(self):
-        self.assertEqual('http://trac.edgewall.org/some/path', self.env.abs_hre
+        abs_href = self.env.abs_href
+        self.assertEqual('http://trac.edgewall.org/some/path', abs_href())
+        self.assertTrue(abs_href is self.env.abs_href)
 
     def test_href(self):
-        self.assertEqual('/some/path', self.env.href())
+        href = self.env.href
+        self.assertEqual('/some/path', href())
+        self.assertTrue(href is self.env.href)
 
     def test_get_version(self):
         """Testing env.get_version"""

Commenting out the setattr​ line in the lazy function should remove the caching, which is what I did to make the test fail before making it pass. That test also revealed that db_exc returns an object with the same id regardless of whether it is decorated with lazy. I didn't dig down into why this is.

[Ryan J Ollos]

Changes from comment:13 can also be found in repos:rjollos.git:t11237.

[anonymous]

self.assertTrue(href is self.env.href) , is there not something like assertInstance? I don't know, that part of the test seems superfluous.

[Ryan J Ollos]

The test is not intended to explicitly check whether the object is an instance of a type. It's testing whether the object returned on the second call is the same object that was returned on the first call. Remove the lazy decorator and the test will fail.

[Ryan J Ollos]

Btw, I'd use ​assertIs if we were utilizing ​unittest2 (or only supporting Python 2.7), but we probably don't want to add another dependency.

[Ryan J Ollos]

Replying to rjollos:

Btw, I'd use ​assertIs if we were utilizing ​unittest2 (or only supporting Python 2.7), but we probably don't want to add another dependency.

Thinking I might just add assertIs in the way that Jun did for assertIn in [11892].

[Ryan J Ollos]

Changes discussed in comment:18 prepared in [c90fb97d/rjollos.git].

If assertIs and assertIn prove to be more widely useful, we could have a subclass of unittest.TestCase that adds these attributes, which all of the test classes would inherit from.

[Jun Omae]

The tests for three methods decorated with @lazy seems good. But, one thing.

    if not hasattr(unittest.TestCase, 'assertIn'):

in [c90fb97d/rjollos.git] should be 'assertIs', not 'assertIn'?

[Ryan J Ollos]

Replying to jomae:

in [c90fb97d/rjollos.git] should be 'assertIs', not 'assertIn'?

Thanks for reviewing. It was funny, I went for a jog just now, and it popped into my head half way through that I probably made that assertIn mistake. I guess I should take the jog before pushing my work from now on!

[Ryan J Ollos]

Committed to 1.0-stable in [11939] and merged to trunk in [11940].

[Ryan J Ollos]

log:rjollos.git:t11245.3 contains a change to EnvironmentStub whereby [trac] base_url is used rather than directly assigning to href and abs_href. I'll wait until milestone:1.0.3 to push the change.

[Jun Omae]

Replying to rjollos:

log:rjollos.git:t11245.3 contains a change to EnvironmentStub whereby [trac] base_url is used rather than directly assigning to href and abs_href.

Yeah, I agree.

[Ryan J Ollos]

I added some additional refactoring of the abs_href method to log:rjollos.git:t11245.3. I don't see a way that Environment.base_url can be None after the changes in [11865], therefore the method can be simplified: [04d68d55/rjollos.git].

[Ryan J Ollos]

Changes from comment:23 and comment:25 committed to 1.0-stable (1.0.3dev) in [13161:13162] and merged to trunk in [13163].