Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#11605 closed task (fixed)

Remove deprecated db parameters

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: general Version:
Severity: normal Keywords:
Cc: leho@… Branch:
Release Notes:
API Changes:
  • The deprecated db parameters have been removed from the signatures of all public methods and functions.
  • Added public method insert_changeset to CachedRepository and deprecated _insert_changeset.
  • The db parameter in the signature of ReportModule.execute_paginated_report is deprecated and may be omitted.
  • The db parameters in the signatures of IEnvironmentSetupParticipant.environment_needs_upgrade and IEnvironmentSetupParticipant.upgrade_environment are deprecated and may be omitted.
  • Environment.get_read_db and the decorators @with_transaction(env) and env.with_transaction() are scheduled for removal in Trac 1.3.1.
  • Environment.get_db_cnx has been removed.
Internal Changes:

Description

Remove deprecated db parameters which were scheduled for removal in Trac 1.1.1, such as trunk/trac/ticket/model.py@12751:86,88-89#L73.

Attachments (0)

Change History (44)

comment:1 by Ryan J Ollos, 10 years ago

ReportModule.execute_paginated_report has a db positional parameter. We would have to change the method signature to fix the issue, however there are no cases of this method being used on trac-hacks:

$ grep -r ".\execute_paginated_report" trachacks.git/
$

comment:2 by Ryan J Ollos, 10 years ago

I guess we should keep the db parameters in IEnvironmentUpgradeParticipant methods, but maybe the parameters should be marked as deprecated: tags/trac-1.0.1/trac/env.py@:76,84#L62.

comment:3 by Ryan J Ollos, 10 years ago

Proposed changes in log:rjollos.git:t11605. The changes address the items raised in comment:1 and comment:2.

comment:4 by Jun Omae, 10 years ago

I don't think it is good for [a9d85e3a/rjollos.git].

I think that no need to remove db parameter from private methods and those methods seems to be private but those are not private for derived classes. Also, the changes lead unnecessary nested transaction context, with_transaction.

Last edited 10 years ago by Jun Omae (previous) (diff)

in reply to:  4 comment:5 by Jun Omae, 10 years ago

.. those are not private for derived classes.

The _insert_changeset method is used in [0068e373/jomae.git] from jomae.git@t10602.1 which have not been pushed yet (#10602).

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

Replying to jomae:

I think that no need to remove db parameter from private methods and those methods seems to be private but those are not private for derived classes.

That's not a good reason to avoid pushing those changes. A private method is exactly that, and your changes would just need to be rebased to adapt to the internal changes.

Also, the changes lead unnecessary nested transaction context, with_transaction.

That may be a good reason to not push the change.

comment:7 by Jun Omae, 10 years ago

CachedRepository.sync() isn't good to git repository with cache, e.g. tracopt.versioncontrol.git (#10602), th:TracPygit2Plugin and my GitPlugin. I think we need overwrite sync method of CachedGitRepository in tracopt.versioncontrol.git and the plugins and I'll do it using CachedRepository._insert_changeset in Trac core and the plugins.

If signature of the method is changed, the change is breaking backward-compatibilities and differences signatures between Trac versions will be unfortunate to me.

Also, I think changes for ReportModule.execute_paginated_report​ is breaking backward-compatibilities. Thing which method is currently not used by the plugins cannot be good reason of those changes which breaks backward-compatibilities. If anyone create cross-version plugins in the future, the changes will annoy the authors.

Last edited 10 years ago by Jun Omae (previous) (diff)

comment:8 by Ryan J Ollos, 10 years ago

If we need to be concerned about changing the signature of _insert_changeset then it should really be a public method. The whole point of making a method private is that it shouldn't be accessed by external components. So the plugins are wrong for accessing that method and expecting a contract to be adhered to in doing so. Practicality prevails though, so I agree that we shouldn't change the method. We should figure out an API change that could be made so that those plugins don't need to access a private method of the class.

As for ReportModule.execute_paginated_report​, we are making the changes on the trunk so breaking compatibilities is allowed. We shouldn't intentionally cause anyone pain, but if the method is unlikely to be used by anyone and we can't see it being used by anyone, then I would prefer to cleanup the API. I think you are being overly pedantic.

I will push [c1d04785/rjollos.git], [766aa7c8/rjollos.git] and [af772265/rjollos.git] and close out this ticket.

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

comment:9 by Ryan J Ollos, 10 years ago

I think we should give [2fb1f6e1/rjollos.git] further consideration. By not changing the method, we are leaving an inconsistency in the API. There are no other public method which get passed a db object, and so the inconsistency will be strange for users of the API. As far as I can see there is no way to ever change this method without breaking backward compatibility. Making the change for Trac 1.2 is just as good as making the change in Trac 1.4.

Any third opinions on the matter?

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

comment:10 by Ryan J Ollos, 10 years ago

Status: newassigned

in reply to:  8 comment:11 by Jun Omae, 10 years ago

If we need to be concerned about changing the signature of _insert_changeset then it should really be a public method. The whole point of making a method private is that it shouldn't be accessed by external components. So the plugins are wrong for accessing that method and expecting a contract to be adhered to in doing so. Practicality prevails though, so I agree that we shouldn't change the method. We should figure out an API change that could be made so that those plugins don't need to access a private method of the class.

Thanks. I think We could add insert_changeset without db parameter and mark _insert_changeset as "deprecated" like the following in this ticket. If the changes is applied, we keep compatibilities and external plugins can easyly detect the method with/without db parameter.

  • trac/versioncontrol/cache.py

    diff --git a/trac/versioncontrol/cache.py b/trac/versioncontrol/cache.py
    index e263a04..cb4fd63 100644
    a b class CachedRepository(Repository):  
    231231                    cset = self.repos.get_changeset(next_youngest)
    232232                    try:
    233233                        # steps 1. and 2.
    234                         self._insert_changeset(db, next_youngest, cset)
     234                        self.insert_changeset(next_youngest, cset)
    235235                    except Exception as e: # *another* 1.1. resync attempt won
    236236                        self.log.warning('Revision %s already cached: %r',
    237237                                         next_youngest, e)
    class CachedRepository(Repository):  
    259259                if feedback:
    260260                    feedback(youngest)
    261261
     262    def insert_changeset(self, rev, cset):
     263        """create revision and node_change records for the given changeset
     264        instance.
     265        """
     266        with self.env.db_transaction as db:
     267            self._insert_changeset(db, rev, cset)
     268
    262269    def _insert_changeset(self, db, rev, cset):
     270        """:deprecated: since 1.1.2, use `insert_changeset` instead and will
     271        be removed in 1.4.
     272        """
    263273        srev = self.db_rev(rev)
    264274        # 1. Attempt to resync the 'revision' table.  In case of
    265275        # concurrent syncs, only such insert into the `revision` table

As for ReportModule.execute_paginated_report​, we are making the changes on the trunk so breaking compatibilities is allowed. We shouldn't intentionally cause anyone pain, but if the method is unlikely to be used by anyone and we can't see it being used by anyone, then I would prefer to cleanup the API. I think you are being overly pedantic.

Yes, I think I'm so pedantic about this. However, Trac developers have kept backward-compatibilities of ReportModule.execute_report​ in [7214] but the method is unused in trac-hacks, https://github.com/rjollos/trachacks/search?q=execute_report.

Version 0, edited 10 years ago by Jun Omae (next)

comment:12 by Ryan J Ollos, 10 years ago

I've refreshed the changes to include the feedback from comment:11: log:rjollos.git:t11605.2. There may be a way to change execute_paginated_report and keep it backwards compatible: [31283efc/rjollos.git]. An additional unit test would be needed to verify proper behavior and the docstring would be improved.

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

comment:13 by Jun Omae, 10 years ago

After [12826], unit tests fail.

======================================================================
ERROR: test_quoted_id_with_var (trac.ticket.tests.report.ReportTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/d5faeed17395bdbbc1f891ac2b80e29c5a06b7f2/py26-sqlite/trac/ticket/tests/report.py", line 127, in test_quoted_id_with_var
    {'USER': 'joe'})
  File "/run/shm/d5faeed17395bdbbc1f891ac2b80e29c5a06b7f2/py26-sqlite/trac/ticket/report.py", line 654, in execute_paginated_report
    sql, args, missing_args = self.sql_sub_vars(sql, args, db)
TypeError: sql_sub_vars() takes exactly 3 arguments (4 given)

...
----------------------------------------------------------------------
Ran 1602 tests in 45.879s

FAILED (errors=9)
make: *** [unit-test] Error 1
make: Leaving directory `/run/shm/d5faeed17395bdbbc1f891ac2b80e29c5a06b7f2/py26-sqlite'

comment:14 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [12826:12831]. I didn't take enough care for testing intermediate revisions to be sure that the tests didn't fail.

comment:15 by Ryan J Ollos, 10 years ago

Here is a case I missed due to grepping for just db: browser:/trunk/trac/env.py@12829:676,684-688#L675.

comment:16 by Ryan J Ollos, 10 years ago

#11639 reported an issue with th:TimingAndEstimationPlugin that is due to removal of the deprecated parameters.

comment:17 by Ryan J Ollos, 10 years ago

Change from comment:15 committed to trunk in [12842].

comment:18 by Ryan J Ollos, 10 years ago

Another deprecated parameter removed in [12854].

comment:19 by Ryan J Ollos, 10 years ago

In [12856:12857], removed support for db parameter from methods decorated with cached.

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

comment:20 by Ryan J Ollos, 10 years ago

Eventually we may want to remove Environment.get_db_cnx, Environment.get_read_db and Environment.with_transaction:

Shall we add to the docstring that the methods will be removed in 1.3.1? Similar changes are scheduled for 1.3.1 already:

in reply to:  20 comment:21 by Peter Suter, 10 years ago

Replying to rjollos:

Shall we add to the docstring that the methods will be removed in 1.3.1?

Sounds like a good idea to me.

comment:22 by Ryan J Ollos, 10 years ago

Thanks for the feedback. Committed in [12970], merged in [12971].

comment:23 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)

in reply to:  20 comment:24 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)

Replying to rjollos:

Eventually we may want to remove Environment.get_db_cnx, Environment.get_read_db and Environment.with_transaction:

Environment.get_db_cnx had already been scheduled for removal in 1.1.1: TracDev/ApiChanges/1.1.2.

I removed it in [13064] and fixed the comment from [12970] in [13065] (followed by record-only merge of that changeset in [13066]).

comment:25 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)

comment:26 by Ryan J Ollos, 10 years ago

According to TracDev/ApiChanges#GuidelinesforAPIchanges, we should log a deprecation warning when calling deprecated functions and methods. We could do it in the following places:

The changes could probably wait until milestone:1.1.3.

comment:27 by Peter Suter, 10 years ago

API Changes: modified (diff)

comment:28 by lkraav <leho@…>, 10 years ago

Cc: leho@… added

comment:29 by Ryan J Ollos, 10 years ago

With all of the API changes being made in 1.3.1, we could take the opportunity to also remove the authname parameter from Environment.get_repository or even remove the get_repository method altogether since it is stated that it has only been left for backward-compatibility: trunk/trac/env.py@13064:551-564#L541. Any suggestions on which direction to go?

comment:30 by Ryan J Ollos, 10 years ago

TracInstall#OtherPythonPackages states that SilverCity and Enscript support is deprecated. Shall we remove these in milestone:1.1.3?

The SilverCity page claims support was dropped in Trac 0.13. Enscript and PHP renderers were deprecated in #8113.

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

comment:31 by Ryan J Ollos, 10 years ago

We could also remove support for mod_python (TracModPython) in milestone:1.1.3. I still see users on running mod_python with Trac 1.0, and it would be good to push them toward mod_wsgi so that we aren't handling bug reports many years from now from users still running mod_python. Thoughts?

in reply to:  29 ; comment:32 by Jun Omae, 10 years ago

Replying to rjollos:

With all of the API changes being made in 1.3.1, we could take the opportunity to also remove the authname parameter from Environment.get_repository or even remove the get_repository method altogether since it is stated that it has only been left for backward-compatibility: trunk/trac/env.py@13064:551-564#L541. Any suggestions on which direction to go?

I agree the removing authname parameter. That parameter is unused since 0.12.

in reply to:  30 ; comment:33 by Jun Omae, 10 years ago

Replying to rjollos:

TracInstall#OtherPythonPackages states that SilverCity and Enscript support is deprecated. Shall we remove these in milestone:1.1.3?

The SilverCity page claims support was dropped in Trac 0.13. Enscript and PHP renderers were deprecated in #8113.

It looks okay to me. That statement for SilverCity and Enscript support has been added to wiki:0.12/TracInstall@8?action=diff before 0.12 is released.

in reply to:  31 ; comment:34 by Jun Omae, 10 years ago

Replying to rjollos:

We could also remove support for mod_python (TracModPython) in milestone:1.1.3. I still see users on running mod_python with Trac 1.0, and it would be good to push them toward mod_wsgi so that we aren't handling bug reports many years from now from users still running mod_python. Thoughts?

Seems that mod_python project is alive. Recently, 3.5.0 is released on Nov-13-2013.

in reply to:  34 ; comment:35 by Ryan J Ollos, 10 years ago

Replying to jomae:

Seems that mod_python project is alive. Recently, 3.5.0 is released on Nov-13-2013.

I guess we should wait and see on that one. I was curious if anyone has run version 3.5.0 with Trac, so I asked on the mailing list: trac-users:1VzLgLT4gDE.

in reply to:  14 ; comment:36 by Peter Suter, 9 years ago

Replying to rjollos:

Committed to trunk in [12826:12831].

After this, running python -m trac.test fails at the functional tests because somehow LegacyParticipant is still enabled(?) from the unit tests.

in reply to:  33 comment:37 by Ryan J Ollos, 9 years ago

Replying to jomae:

Replying to rjollos:

TracInstall#OtherPythonPackages states that SilverCity and Enscript support is deprecated. Shall we remove these in milestone:1.1.3?

The SilverCity page claims support was dropped in Trac 0.13. Enscript and PHP renderers were deprecated in #8113.

It looks okay to me. That statement for SilverCity and Enscript support has been added to wiki:0.12/TracInstall@8?action=diff before 0.12 is released.

Dedicated ticket is now #11795.

in reply to:  32 comment:38 by Ryan J Ollos, 9 years ago

Replying to jomae:

Replying to rjollos:

With all of the API changes being made in 1.3.1, we could take the opportunity to also remove the authname parameter from Environment.get_repository or even remove the get_repository method altogether since it is stated that it has only been left for backward-compatibility: trunk/trac/env.py@13064:551-564#L541. Any suggestions on which direction to go?

I agree the removing authname parameter. That parameter is unused since 0.12.

Removed in [13193], see comment:12:ticket:11703.

in reply to:  36 ; comment:39 by Ryan J Ollos, 9 years ago

Replying to psuter:

Replying to rjollos:

Committed to trunk in [12826:12831].

After this, running python -m trac.test fails at the functional tests because somehow LegacyParticipant is still enabled(?) from the unit tests.

Yeah I can reproduce. Good catch. I hadn't seen the issue running make test (or equivalently python ./trac/test.py --skip-functional-tests followed by python trac/tests/functional/__init__.py -v).

The issue goes away when the two methods are commented out: trunk/trac/tests/env.py@:134,156#L133, but I'm not sure why the LegacyParticipants are enabled for the functional tests.

in reply to:  39 comment:40 by Peter Suter, 9 years ago

Replying to rjollos:

but I'm not sure why the LegacyParticipants are enabled for the functional tests.

By default all components in the trac packages are enabled, except trac.test.* (since #11421) but trac.tests.env.LegacyParticipant doesn't match that (test vs. tests).

in reply to:  36 comment:41 by Peter Suter, 9 years ago

Replying to psuter:

After this, running python -m trac.test fails at the functional tests because somehow LegacyParticipant is still enabled(?) from the unit tests.

I created #11815.

in reply to:  19 comment:42 by Ryan J Ollos, 9 years ago

Replying to rjollos:

In [12856:12857], removed support for db parameter from methods decorated with cached.

In [13461], removed import that should have been removed in [12856].

in reply to:  17 comment:43 by Ryan J Ollos, 9 years ago

Replying to rjollos:

Change from comment:15 committed to trunk in [12842].

Change to EnvironmentStub.get_known_users in [13465].

in reply to:  35 comment:44 by Ryan J Ollos, 9 years ago

Replying to rjollos:

Replying to jomae:

Seems that mod_python project is alive. Recently, 3.5.0 is released on Nov-13-2013.

I guess we should wait and see on that one. I was curious if anyone has run version 3.5.0 with Trac, so I asked on the mailing list: trac-users:1VzLgLT4gDE.

The deprecation warning was removed by a wiki editor in TracModPython@165. I guess we will continue to support it unless someone has additional input.

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.