Code review

[Steffen Hoffmann]

As recent development has been tracked in #6783, but going totally out-of-scope now, this ticket is meant to collect references and additional comment regarding this ongoing effort for tracdiscussion-0.9.

[Steffen Hoffmann]

In 13890:

DiscussionPlugin: Script header/imports cleanup, refs #11706.

More PEP8 changes (doc-string quotation placement, line-wrap), comment review,
and complete preparation of URLs to messages as anchors in the corresponding
topic (parent resource) page by get_resource_url() for the discussion
Trac Resource domain as planned in [13882].

[Steffen Hoffmann]

In 13891:

DiscussionPlugin: Add license headers to python scripts making license more obvious as per Trac plugin coding recommendations, refs #11706.

[Steffen Hoffmann]

In 13893:

DiscussionPlugin: Add license header to Genshi HTML templates making license more obvious here too, refs #11706.

[Steffen Hoffmann]

In 13909:

DiscussionPlugin: Create automated test harness for this plugin, refs #11706.

Arrange unittest infrastructure for all modules, including test methods
with a couple of component-initialization-only tests for now.

Tests can be run either as the full suite:

$> PYTHONPATH=. python setup.py test

or by just running a single test module:

$> PYTHONPATH=. python -m tracdiscussion.tests.api

[Steffen Hoffmann]

In 13910:

DiscussionPlugin: Cleanup spam filter adapter component, refs #11706.

[Steffen Hoffmann]

In 13915:

DiscussionPlugin: Move db access methods into separate module, refs #11706.

This is the initial split as done to other plugins before for improved
maintainability of db schema-specific code. Cutting method count per class
as well as overall lines per class down to more manageable size will help
anyway.

[Steffen Hoffmann]

In 13916:

DiscussionPlugin: Correct issues with tag retrieval, refs #756 and #11706.

Resource construction must happen before its use in a tag provider method.
An additional private method enables code deduplication for forum data
retrieval.

[Ryan J Ollos]

In [13916]: extracting the discussion string to the realm attribute is a good idea. All IResourceManager and model classes in the Trac core should follow this pattern. Only a few do, such as the WikiPage model: ​t:browser:/tags/trac-1.0.1/trac/wiki/model.py?marks=33#L30, but even in that case you can see instances where self.realm isn't used to construct Resource objects: ​t:browser:/tags/trac-1.0.1/trac/wiki/model.py?marks=43#L30. I started working on fixing this issue in the Trac core a few weeks back but never got around to finishing. Maybe I will find time to pick that up again this weekend.

[Ryan J Ollos]

forum_cols was added as an attribute of the DiscussionDb class, but the _get_forum method which references the forum_cols attribute via the call self.forum_cols was moved to DiscussionApi. It looks like that might result in an AttributeError.

[Steffen Hoffmann]

In 13917:

DiscussionPlugin: Rework other single item getter methods, refs #11706.

While leaving single column references in api module looks like imperfect
isolation of schema-related information, it seems reasonable to at least
maintain table column lists in model module.

[Steffen Hoffmann]

Replying to rjollos:

forum_cols was added as an attribute of the DiscussionDb class, but the _get_forum method which references the forum_cols attribute via the call self.forum_cols was moved to DiscussionApi. It looks like that might result in an AttributeError.

I've been uneasy about this too, however the class inheritance of DiscussionDb blends all attributes nicely to DiscussionApi, so I just did the same for other column name tuples as well.

[Ryan J Ollos]

I see. I didn't see that the base class was DiscussionDb, in which case I don't see any problem with it.

[Steffen Hoffmann]

Replying to rjollos:

I see. I didn't see that the base class was DiscussionDb, in which case I don't see any problem with it.

Thanks for code-review anyway. Being a bit suspicious doesn't hurt.

[Steffen Hoffmann]

Replying to rjollos:

In [13916]: extracting the discussion string to the realm attribute is a good idea.

Interesting. While I did this change without knowing that pattern from other resource managers, mainly for reducing string repetitions, I see the point in making it a common class attribute.

[Ryan J Ollos]

Replying to hasienda:

Replying to rjollos:

In [13916]: extracting the discussion string to the realm attribute is a good idea.

Interesting. While I did this change without knowing that pattern from other resource managers, mainly for reducing string repetitions, I see the point in making it a common class attribute.

I went ahead and made a ticket: ​t:#11609.

[Steffen Hoffmann]

In 13919:

DiscussionPlugin: More PEP8 method cleanup, refs #11706.

Adding unit tests for reviewed methods we've reached 32 tests, all passed.

[Steffen Hoffmann]

In 13920:

DiscussionPlugin: Review more complex methods, refs #11706.

Apart from usual PEP8 issues some SQL queries could be replaced with existing
method calls for robustness and saving lines in the module.
Remove debug logging, where resource changes in context are obvious anyway.

[Steffen Hoffmann]

In 13921:

DiscussionPlugin: Stop requests lacking view permission to whole realm, refs #756 and #11706.

[Ryan J Ollos]

Could you use context.req.perm.require('DISCUSSION_VIEW') in [13921]?: ​trac:browser:/tags/trac-0.11/trac/perm.py?marks=543#L543.

[Steffen Hoffmann]

Replying to rjollos:

Could you use context.req.perm.require('DISCUSSION_VIEW') in [13921]?

You would prefer the explicit method call, wouldn’t you? No problem for me as both notations should be nearly equivalent, even technically IIRC.

[Steffen Hoffmann]

In 13922:

DiscussionPlugin: Don't need to custom permission error, refs #756 and #11706.

This would be a reason to use the previous pattern. Thanks for the hint.

[Steffen Hoffmann]

In [13923]:

DiscussionPlugin: Do not insert unescaped values into SQL query string.

This issue in model._get_items() went through on first review for [13919]. Luckily seeing the same thing done right in get_topics pointed it out. In a similar notion unregistered users are retrieved by set difference instead of iteration over topic['subscribers'] in _prepare_topic() too.

[Steffen Hoffmann]

In 13925:

DiscussionPlugin: More PEP8 (excessive white-space, line-wrap), simplifications and style changes, refs #11706.

[Steffen Hoffmann]

In 13926:

DiscussionPlugin: Consolidate remaining API methods for getting db content, refs #11706.

This includes a fix for api.get_users, that was broken in [13925].

[Steffen Hoffmann]

In 13929:

DiscussionPlugin: Refactor and split methods into util modul functions, refs #11706.

49 tests document both, the code itself as well as review progress.

[Steffen Hoffmann]

In 13930:

DiscussionPlugin: Prevent AttributeError when using API methods with pristine Trac context, refs #8637 and #11706.

This shall fix the issue reported for email2trac (yet untested) as well as
contribute to code robustness in general.

[Steffen Hoffmann]

In 13935:

DiscussionPlugin: Complete db access API methods, refs #11706.

A missing import from previous modifications has been spotted on creation of
corresponding tests.

[Steffen Hoffmann]

In 13936:

DiscussionPlugin: Reorganize search source code, refs #756 and #11706.

In fixing discussion resource links this is a follow-up to [13914].

The remains after moving db access code to model don't justify to keep
search as a separate module. Added search should help preventing further
regressions for this functionality.

[Steffen Hoffmann]

In 13944:

DiscussionPlugin: Review wiki macros and syntax provider code, refs #756 and #11706.

This includes a wide range of changes from adding another db access method to
api over moving SQL into model module to correcting resource ID as
required for fine-grained permission support.

Adding the missing import from trac.resource as follow-up to [9640] suggests
rare use of discussion WikiMacros so far.

[Steffen Hoffmann]

In 13945:

DiscussionPlugin: Extend use of new attribute getter api method, refs #11706.

Preventing TypeError in case of invalid ID argument is required for this
use, and doesn't hurt for others anyway.

[Ryan J Ollos]

In 15548:

0.10dev: Pass format_to_oneliner_no_links in template data

Refs #11706.

[Ryan J Ollos]

In 15549:

0.10dev: Fix typo in [15548]

Refs #11706, #12762.

[Ryan J Ollos]

In 16012:

0.10dev: Fix incorrect string formatting and comparisons, missing imports

Refs #11706.

[Ryan J Ollos]

In 16013:

1.2dev: Fix incorrect string formatting and comparisons, missing imports

Merge of r16012 from 1.0 branch.

Refs #11706, #12971.

[Ryan J Ollos]

In 16014:

0.10.0: Bump version from 0.10dev

Refs #11706.

[Ryan J Ollos]

Going forward only major fixes will be applied to the 1.0 branch. Code review and improvements will continue in #12971.