Remove deprecated code (Trac 1.3.1)

[Ryan J Ollos]

This ticket lists code that is scheduled for removal in Trac 1.3.1.

• Removal of ipnr attribute in attachment and wiki modules: #9612.
• Remove deprecated methods in trac.wiki.formatter: #11539.
• Remove deprecated notification code (comment:21:ticket:3517).
• Remove svn_fs.py​ and svn_prop.py​ (modules moved in #10712).
• Remove since strings from code comments and option documentation for features added in Trac < 1.0 (as in [13889])

Other changes:

• Replace use of ​optparse with ​argparse ⇒ #12148.
• Replace class with a namedtuple: tags/trac-1.1.4/tracopt/versioncontrol/git/PyGIT.py@:244-249#L240​ ⇒ #12027.
• SpamFilter: Require Pillow ≥ 2.0.0 (comment:1:ticket:12133).

Building the list will be easier to do as we develop, and the changes can then be removed at the start of the milestone.

TracDev/ApiChanges/1.3 should be updated with the API changes before this ticket is closed.

[Peter Suter]

• Notify, NotifyEmail, TicketNotifyEmail and BatchTicketNotifyEmail ([13578])
• get_ticket_notification_recipients ([13609])

[Ryan J Ollos]

I was going to make a list of code to remove in this ticket, but it seemed easier to just prepare a branch. I'll periodically rebase the branch against the trunk and commit the changes at the start of milestone:1.3.1. The first revision of the changes will be posted here after #11913 is committed at the start of milestone:1.0.4.

[Ryan J Ollos]

Replying to rjollos:

I was going to make a list of code to remove in this ticket, but it seemed easier to just prepare a branch.

Some changes have been staged in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1. I haven't removed all of the deprecated code, in particular the notification module code. The upgrade step for removing the ipnr columns from the database also hasn't been written yet (#11872 could help with that).

[Ryan J Ollos]

Compatibility code can be removed in 1.3.1 if Python 2.6 support is dropped: [ba3d34ac/rjollos.git].

[Ryan J Ollos]

Rebased and additional changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.2.

[Ryan J Ollos]

Rebased and additional changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.4.

[Jun Omae]

unit tests failing.

...
  File "./trac/test.py", line 391, in suite
    import tracopt.versioncontrol.svn.tests
  File "/run/shm/6d672f85ac8080ca247cc91cfb140c339863b711/py27-sqlite/tracopt/versioncontrol/svn/tests/__init__.py", line 16, in <module>
    from tracopt.versioncontrol.svn.tests import svn_fs
  File "/run/shm/6d672f85ac8080ca247cc91cfb140c339863b711/py27-sqlite/tracopt/versioncontrol/svn/tests/svn_fs.py", line 42, in <module>
    from trac.versioncontrol import svn_fs, svn_prop
ImportError: cannot import name svn_fs
make: *** [unit-test] Error 1

The svn_fs and svn_prop in trac.versioncontrol is still used from tracopt/versioncontrol/svn/tests/svn_fs.py.

$ git grep -w 'import.*svn_\(fs\|prop\)'
tracopt/versioncontrol/svn/tests/__init__.py:from tracopt.versioncontrol.svn.tests import svn_fs
tracopt/versioncontrol/svn/tests/svn_fs.py:from trac.versioncontrol import svn_fs, svn_prop

[Jun Omae]

Replying to jomae:

The svn_fs and svn_prop in trac.versioncontrol is still used from tracopt/versioncontrol/svn/tests/svn_fs.py.

Ouch. That code is introduced in [12995] by me. I'm wrong. After 1.0.6 release, I'll fix it on 1.0-stable and trunk.

[Ryan J Ollos]

Rebased and additional changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.5.

[Jun Omae]

Replying to jomae:

Replying to jomae:

The svn_fs and svn_prop in trac.versioncontrol is still used from tracopt/versioncontrol/svn/tests/svn_fs.py.

Ouch. That code is introduced in [12995] by me. I'm wrong. After 1.0.6 release, I'll fix it on 1.0-stable and trunk.

Fixed in [14101] and merged to trunk in [14102].

[Ryan J Ollos]

Should we remove the deprecated cnum (#7145) from Ticket.save_changes in 1.3.1?: tags/trac-1.1.5/trac/ticket/model.py@:313-314#L306​.

[Jun Omae]

I agree to removing cnum from Ticket.save_changes() in 1.3.1.

[Ryan J Ollos]

Rebased and additional changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.6.

[Ryan J Ollos]

show_email_addresses was deprecated from the chrome data dictionary in #11474 / [14151#file2] and will be removed in the next set of proposed changes.

[Ryan J Ollos]

[3751] may have some compatibility code to be removed, which we might even be able to do on the currently trunk. We'd have to replace HTTPInternalError with HTTPInternalServerError in the half-dozen or so instances that it appears in the Trac codebase. TracDev/Exceptions should be updated if the change is made.

[Ryan J Ollos]

#12132 closed as a duplicate.

Latest changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.7, incorporating changes from #12132 and comment:21.

[Ryan J Ollos]

It appears the following built-ins don't need to be passed in the data dictionary any longer: tags/trac-1.1.6/trac/web/chrome.py@:597-598,622,625#L594​.

I've also considered that it would be generally useful to pass functools, itertools and operator in the chrome data dictionary. Previously we've discussed passing presentation (comment:15:ticket:8989).

[Ryan J Ollos]

Changes rebase and deprecated notification classes removed in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.8.

[Ryan J Ollos]

Latest changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.9 includes a fix for #12205.

[Ryan J Ollos]

DONE Consider bumping Pygments minimum version to at least 1.0 and remove regression test added in #7705.

[Ryan J Ollos]

DONE Rebase staged changes after changes in #12206 are committed to the trunk. TicketNotifyEmail.obfuscate_email will be removed.

[Ryan J Ollos]

[1a9ad113/rjollos.git] attempts a minor simplification of Ticket.save_changes. Is there any reason to wait until the end of the method to update self.values['changetime']? I've considered that an exception might be raised in the method, but haven't come up with a scenario in which the change would cause a problem.

[Ryan J Ollos]

[de431734/rjollos.git] replaces property() calls with the more readable @property decorator.

Latest changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.11.

[Ryan J Ollos]

Expanding on the changes in #11565 and #11587, I propose in [327fadb1/rjollos.git] to remove the deprecated systeminfo attribute, deprecate ISystemInfoProvider and add a lazily-evaluated system_info property.

This would finally cleanup the Environment class by:

• Eliminating get_systeminfo which has a name too similar to get_system_info.
• Forcing plugins to implement the ISystemInfoProvider interface rather than appending to systeminfo.

Rebased changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.12.

[Ryan J Ollos]

Rebased changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.13. Environment.get_repository has been removed.

[Ryan J Ollos]

Since [4559], the version info is retrieved from an attribute if Pygments is installed from source, or more specifically if the version can't be retrieved from package info: tags/trac-1.0.9/trac/mimeview/pygments.py@:89-91#L85​.

I'm not entirely sure what installed from source means, but I assume it means installed in dev mode.

It looks like we can remove the complexity. I've evaluated with Pygments 2.1dev (which is post-2.1, so it appears the version hasn't been bumped after the 2.1 release) and 1.0 (minimum Pygments version bumped to 1.0 in comment:29).

Pygments installed in dev mode from tip of Hg repository:

>>> import pygments
>>> from trac.util import get_pkginfo
>>> pkginfo = get_pkginfo(pygments)
>>> pkginfo['version']
'2.1.dev20160119'
>>> pygments.__version__
'2.1'

​Pygments 1.0 installed in dev mode.

>>> import pygments
>>> from trac.util import get_pkginfo
>>> pkginfo = get_pkginfo(pygments)
>>> pkginfo['version']
'1.0'
>>> pygments.__version__
'1.0'

[Ryan J Ollos]

Changes rebased in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.14.

[Christian Boos]

Changes look good, I'll probably take a second, more detailed look when 1.3.x starts, but so far I like what I've seen.

[Ryan J Ollos]

Additional changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.15.

[Ryan J Ollos]

In Python 2.7 and later, ​ZipFile can be used as a context manager. Proposed changes in [221c8b5d7/rjollos.git].

[Ryan J Ollos]

First set of changes committed in [14880:14895].

Latest proposed changes in log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.17. I'll work on adding the database upgrade for [e15f0f3d/rjollos.git].

[Jun Omae]

In [14879], 2.6 with trunk has been removed from matrix build on trunk/.travis.yml. We should remove Python26 and Python26-x64 with trunk from matrix build of trunk/.appveyor.yml as well as .travis.yml.

[Jun Omae]

Functional tests with [94f40c9a9/rjollos.git] failed. See 94f40c9a9-py27-sqlite.log​.

FAILED (failures=15)

[Ryan J Ollos]

.appveyor.yml modified in r14896, but cleanup of trunk/contrib/appveyor.ps1 is still needed. Also, .appveyor.yml probably needs to be modified on branches/1.2-stable​ (comment:35:ticket:12120).

[Ryan J Ollos]

Additional changes in [14903:14923,14925:14926].

[Ryan J Ollos]

Additional changes in [14930:14941]. I'm discarding [94f40c9a9/rjollos.git] and [e15f0f3d/rjollos.git] will be committed in #9612.

[Christian Boos]

Replying to Ryan J Ollos:

cleanup of trunk/contrib/appveyor.ps1 is still needed.

Done in r14955, I think. Don't hesitate to propose further clean-ups if you think I missed something.

[Ryan J Ollos]

Additional change in r14973, r15060.

[Ryan J Ollos]

#12539 is a regression in r14888.

[Ryan J Ollos]

EnvironmentStub.reset_db was implemented in [8194#file4], prior to the introduction of database transaction context managers. I can see the case for calling rollback in the original implementation​, but I'm less sure that call is necessary now that transaction context managers are used: trunk/trac/test.py@15050:377#L366​. Shouldn't the content manager rollback any transactions when __exit__ is called?

trac/test.py

@@ -373 +373 @@
         tables = []
         dbm = DatabaseManager(self)
         try:
-            with self.db_transaction as db:
-                db.rollback()  # make sure there's no transaction in progress
-                # check database version
-                db_version = dbm.get_database_version()
+            db_version = dbm.get_database_version()
         except (TracError, self.db_exc.DatabaseError):
             pass
         else:
@@ -389 +386 @@
 
         if not tables:
             dbm.init_db()
-            # we need to make sure the next get_db_cnx() will re-create
+            # Make sure the next db_query()/db_transaction() will create
             # a new connection aware of the new data model - see #8518.
             if self.dburi != 'sqlite::memory:':
                 dbm.shutdown()

[Jun Omae]

Yeah. The changes look good to me. I agree no need to call rollback because Environment.get_db_cnx() has been removed in 1.3.1dev.

[Ryan J Ollos]

Thanks for feedback. Committed to trunk in r15065.

[Ryan J Ollos]

r15101 uses ​NullHandler, available since Python 2.7.

[Ryan J Ollos]

r15112 normalizes Environment.path in the initializer.

[Ryan J Ollos]

In r15114, fixed upgrade failures due to r14983.

[Ryan J Ollos]

Added attachments_dir attribute to Environment in r15115.

[Ryan J Ollos]

Removed unused attribute in r15118.

[Ryan J Ollos]

I've considered decoupling the query and batch module by implementing IRequestFilter in BatchModifyModule: rjollos.git:decouple_batch_and_query_modules.

I'm still considering whether it's a good idea, and welcome feedback on that.

[Peter Suter]

I've considered decoupling the query and batch module by implementing IRequestFilter in BatchModifyModule: rjollos.git:decouple_batch_and_query_modules.

I don't know IRequestFilter that well, so I can't comment on that, but have you considered something like #12156?

[Jun Omae]

Replying to Ryan J Ollos:

I've considered decoupling the query and batch module by implementing IRequestFilter in BatchModifyModule: rjollos.git:decouple_batch_and_query_modules.

I'm still considering whether it's a good idea, and welcome feedback on that.

I think it's good to use BatchModifyModule with IRequestFilter rather than self.env[BatchModifyModule] in QueryModule. Also, I consider it's good to apply it to 1.0-stable.

[Ryan J Ollos]

Replying to psuter:

I don't know IRequestFilter that well, so I can't comment on that, but have you considered something like #12156?

That looks like it's worth reviewing since you've done all the work already. I'll test it out and we can discuss whether it should be included in 1.3.1.

[Ryan J Ollos]

Replying to Jun Omae:

I think it's good to use BatchModifyModule with IRequestFilter rather than self.env[BatchModifyModule] in QueryModule. Also, I consider it's good to apply it to 1.0-stable.

I rebased the changes on 1.0-stable and created ticket #12591.