Edgewall Software
Modify

Opened 5 years ago

Closed 3 years ago

Last modified 3 months ago

#11901 closed task (fixed)

Remove deprecated code (Trac 1.3.1)

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.1
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Removed deprecated functions, classes, attributes and methods.

API Changes:

Description (last modified by 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:

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.

Attachments (1)

94f40c9a9-py27-sqlite.log (42.6 KB ) - added by Jun Omae 3 years ago.

Download all attachments as: .zip

Change History (78)

comment:1 by Peter Suter, 5 years ago

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

comment:2 by Ryan J Ollos, 5 years ago

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.

comment:3 by Ryan J Ollos, 5 years ago

Description: modified (diff)

in reply to:  2 comment:4 by Ryan J Ollos, 5 years ago

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

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

comment:5 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:6 by Ryan J Ollos, 5 years ago

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

comment:7 by Ryan J Ollos, 5 years ago

comment:8 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:9 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:10 by Ryan J Ollos, 5 years ago

Description: modified (diff)
Milestone: next-major-releases1.3.1
Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:11 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:12 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:13 by Jun Omae, 5 years ago

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

in reply to:  13 ; comment:14 by Jun Omae, 5 years ago

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.

comment:15 by Ryan J Ollos, 5 years ago

in reply to:  14 comment:16 by Jun Omae, 5 years ago

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

comment:17 by Ryan J Ollos, 4 years ago

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.

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

comment:18 by Jun Omae, 4 years ago

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

comment:19 by Ryan J Ollos, 4 years ago

comment:20 by Ryan J Ollos, 4 years ago

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.

comment:21 by Ryan J Ollos, 4 years ago

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

comment:22 by Ryan J Ollos, 4 years ago

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

comment:23 by Ryan J Ollos, 4 years ago

Description: modified (diff)

comment:24 by Ryan J Ollos, 4 years ago

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

comment:25 by Ryan J Ollos, 4 years ago

Description: modified (diff)

comment:26 by Ryan J Ollos, 4 years ago

Description: modified (diff)

comment:27 by Ryan J Ollos, 4 years ago

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

comment:28 by Ryan J Ollos, 4 years ago

comment:29 by Ryan J Ollos, 4 years ago

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

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

comment:30 by Ryan J Ollos, 4 years ago

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

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

comment:31 by Ryan J Ollos, 4 years ago

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

comment:32 by Ryan J Ollos, 4 years ago

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

comment:33 by Ryan J Ollos, 4 years ago

Description: modified (diff)

comment:34 by Ryan J Ollos, 4 years ago

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.

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

comment:35 by Ryan J Ollos, 4 years ago

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

comment:36 by Ryan J Ollos, 4 years ago

Since [4559], the version info is retrieved from an attribute if Pygments is installed from source: 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'

In fact we are throwing away potentially important info by showing pygments.__version__.

Version 3, edited 4 years ago by Ryan J Ollos (previous) (next) (diff)

comment:38 by Christian Boos, 4 years ago

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.

comment:40 by Ryan J Ollos, 4 years ago

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

comment:41 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

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

comment:42 by Jun Omae, 3 years ago

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.

by Jun Omae, 3 years ago

Attachment: 94f40c9a9-py27-sqlite.log added

comment:43 by Jun Omae, 3 years ago

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

FAILED (failures=15)

comment:44 by Ryan J Ollos, 3 years ago

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

comment:45 by Ryan J Ollos, 3 years ago

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

comment:46 by Ryan J Ollos, 3 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

in reply to:  44 comment:47 by Christian Boos, 3 years ago

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.

comment:48 by Ryan J Ollos, 3 years ago

Additional change in r14973, r15060.

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

comment:49 by Ryan J Ollos, 3 years ago

#12539 is a regression in r14888.

comment:50 by Ryan J Ollos, 3 years ago

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

    diff --git a/trac/test.py b/trac/test.py
    index d67a619..02b5129 100755
    a b class EnvironmentStub(Environment):  
    373373        tables = []
    374374        dbm = DatabaseManager(self)
    375375        try:
    376             with self.db_transaction as db:
    377                 db.rollback()  # make sure there's no transaction in progress
    378                 # check database version
    379                 db_version = dbm.get_database_version()
     376            db_version = dbm.get_database_version()
    380377        except (TracError, self.db_exc.DatabaseError):
    381378            pass
    382379        else:
    class EnvironmentStub(Environment):  
    389386
    390387        if not tables:
    391388            dbm.init_db()
    392             # we need to make sure the next get_db_cnx() will re-create
     389            # Make sure the next db_query()/db_transaction() will create
    393390            # a new connection aware of the new data model - see #8518.
    394391            if self.dburi != 'sqlite::memory:':
    395392                dbm.shutdown()

comment:51 by Jun Omae, 3 years ago

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.

comment:52 by Ryan J Ollos, 3 years ago

Thanks for feedback. Committed to trunk in r15065.

comment:53 by Ryan J Ollos, 3 years ago

r15101 uses NullHandler, available since Python 2.7.

comment:54 by Ryan J Ollos, 3 years ago

r15112 normalizes Environment.path in the initializer.

comment:55 by Ryan J Ollos, 3 years ago

In r15114, fixed upgrade failures due to r14983.

comment:56 by Ryan J Ollos, 3 years ago

Added attachments_dir attribute to Environment in r15115.

comment:57 by Ryan J Ollos, 3 years ago

Removed unused attribute in r15118.

comment:58 by Ryan J Ollos, 3 years ago

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.

in reply to:  58 ; comment:59 by Peter Suter, 3 years ago

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?

in reply to:  58 ; comment:60 by Jun Omae, 3 years ago

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.

in reply to:  59 comment:61 by Ryan J Ollos, 3 years ago

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.

in reply to:  60 comment:62 by Ryan J Ollos, 3 years ago

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.

comment:63 by Ryan J Ollos, 3 years ago

Description: modified (diff)

comment:64 by Ryan J Ollos, 3 years ago

Description: modified (diff)

SpamFilter 1.2.0dev requires Pillow ≥ 2: r15231.

comment:65 by Ryan J Ollos, 3 years ago

rjollos.git:t11901_dict_comprehensions replaces uses of dict() with dict comprehensions.

in reply to:  65 ; comment:66 by Ryan J Ollos, 3 years ago

Replying to Ryan J Ollos:

rjollos.git:t11901_dict_comprehensions replaces uses of dict() with dict comprehensions.

Set comprehensions and set literals are utilized in [e1779798/rjollos.git]. New branch: log:rjollos.git:t11901_dict_comprehensions.1.

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

comment:67 by Ryan J Ollos, 3 years ago

Committed to trunk in r15259.

comment:68 by Jun Omae, 3 years ago

After [14882], SQL defined in report is executed with self.env.db_transaction. Before the changes, SQL is executed with self.env.db_query. Why?

in reply to:  68 comment:69 by Ryan J Ollos, 3 years ago

Replying to Jun Omae:

Why?

Looks like a mistake.

comment:70 by Ryan J Ollos, 3 years ago

Regression fixed in r15358. Thanks for spotting.

in reply to:  66 comment:71 by Ryan J Ollos, 3 years ago

Replying to Ryan J Ollos:

Set comprehensions and set literals are utilized in [e1779798/rjollos.git]. New branch: log:rjollos.git:t11901_dict_comprehensions.1.

Additional changes committed in [15614:15618].

comment:72 by Ryan J Ollos, 3 years ago

The options ambiguous_char_width, ticket_subject_template and batch_subject_template are defined on the TicketNotificationSystem class, which is an artifact of the old notification system: tags/trac-1.2/trac/ticket/notification.py@:63,72,77#L42. Now that the legacy code is removed the values are only used in TicketFormatter. I think it makes sense to make the Options class attributes of TicketFormatter, so that if TicketFormatter is replaced by a plugin we don't have unused Options. Failure to move those when removing the legacy notification code was an oversight by me.

Proposed changes in [7c7918bc/rjollos.git].

comment:73 by Ryan J Ollos, 3 years ago

comment:72 changes committed to trunk in r15669.

comment:74 by Ryan J Ollos, 3 years ago

[diff] tab_width was deprecated in r2409, so looks like we can remove:

  • trac/versioncontrol/web_ui/changeset.py

    commit 8cdfa58463cf1f37e5b1ed696c7022d99d0ccb0f
    Author: Ryan J Ollos <ryan.j.ollos@gmail.com>
    Date:   Thu Apr 6 14:12:02 2017 -0700
    
        Remove tab_width
    
    diff --git a/trac/versioncontrol/web_ui/changeset.py b/trac/versioncontrol/web_ui/changeset.py
    index f15eb2fe9..e7cea7920 100644
    a b class ChangesetModule(Component):  
    559559                context = options.get('contextlines', 3)
    560560                if context < 0 or options.get('contextall'):
    561561                    context = None
    562                 tabwidth = self.config['diff'].getint('tab_width') or \
    563                            self.config['mimeviewer'].getint('tab_width', 8)
     562                tabwidth = self.config.getint('mimeviewer', 'tab_width', 8)
    564563                ignore_blank_lines = options.get('ignoreblanklines')
    565564                ignore_case = options.get('ignorecase')
    566565                ignore_space = options.get('ignorewhitespace')

comment:75 by Ryan J Ollos, 3 years ago

comment:74 change committed to trunk in r15748.

comment:76 by Ryan J Ollos, 2 years ago

Environment.attachments_dir was added in r15115 for Trac 1.3.1. The changes proposed for #11675 eliminate the property, and those changes won't land until at least Trac 1.5.1. I'd like to revert the addition of Environment.attachments_dir in anticipation of those changes: [24dbd6c30/rjollos.git].

in reply to:  21 comment:77 by Ryan J Ollos, 3 months ago

Replying to 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.

r14917 Documented in TracDev/Exceptions@22.

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 as closed 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.