Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#11245 closed enhancement (fixed)

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

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: low Milestone: 1.0.2
Component: general Version: 1.0-stable
Severity: normal Keywords: environment lazy refactoring
Cc: olemis+trac@… Branch:
Release Notes:

Refactored 3 methods of the Environment class to use the lazy decorator.

API Changes:
Internal Changes:

Description

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.

Attachments (0)

Change History (26)

comment:1 by Ryan J Ollos, 7 years ago

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

comment:2 by Olemis Lang <olemis+trac@…>, 7 years ago

Cc: olemis+trac@… added

comment:3 by Jun Omae, 7 years ago

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.

comment:4 by Peter Suter, 7 years ago

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?

in reply to:  3 comment:5 by Ryan J Ollos, 7 years ago

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.

in reply to:  4 comment:6 by Ryan J Ollos, 7 years ago

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.

comment:7 by Ryan J Ollos, 7 years ago

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.

comment:8 by Ryan J Ollos, 7 years ago

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.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

in reply to:  8 comment:9 by Ryan J Ollos, 7 years ago

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.

comment:10 by Ryan J Ollos, 7 years ago

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

comment:11 by Ryan J Ollos, 7 years ago

Keywords: refactoring added
Milestone: 1.0.2
Owner: set to Ryan J Ollos
Release Notes: modified (diff)
Status: newassigned

comment:12 by Ryan J Ollos, 7 years ago

Resolution: fixed
Status: assignedclosed

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

comment:13 by Ryan J Ollos, 7 years ago

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

    diff --git a/trac/tests/env.py b/trac/tests/env.py
    index f7d5595..35bd465 100644
    a b class EnvironmentTestCase(unittest.TestCase):  
    6565        self.env.shutdown() # really closes the db connections
    6666        shutil.rmtree(self.env.path)
    6767
     68    def test_db_exc(self):
     69        db_exc = self.env.db_exc
     70        self.assertTrue(hasattr(db_exc, 'IntegrityError'))
     71        self.assertTrue(db_exc is self.env.db_exc)
     72
    6873    def test_abs_href(self):
    69         self.assertEqual('http://trac.edgewall.org/some/path', self.env.abs_hre
     74        abs_href = self.env.abs_href
     75        self.assertEqual('http://trac.edgewall.org/some/path', abs_href())
     76        self.assertTrue(abs_href is self.env.abs_href)
    7077
    7178    def test_href(self):
    72         self.assertEqual('/some/path', self.env.href())
     79        href = self.env.href
     80        self.assertEqual('/some/path', href())
     81        self.assertTrue(href is self.env.href)
    7382
    7483    def test_get_version(self):
    7584        """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.

comment:14 by Ryan J Ollos, 7 years ago

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

comment:15 by anonymous, 7 years ago

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

comment:16 by Ryan J Ollos, 7 years ago

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.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:17 by Ryan J Ollos, 7 years ago

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.

in reply to:  17 comment:18 by Ryan J Ollos, 7 years ago

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

comment:19 by Ryan J Ollos, 7 years ago

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.

comment:20 by Jun Omae, 7 years ago

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'?

in reply to:  20 comment:21 by Ryan J Ollos, 7 years ago

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!

comment:22 by Ryan J Ollos, 7 years ago

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

comment:23 by Ryan J Ollos, 6 years ago

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.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

in reply to:  23 comment:24 by Jun Omae, 6 years ago

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.

comment:25 by Ryan J Ollos, 6 years ago

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

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:26 by Ryan J Ollos, 6 years ago

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.