#11605 closed task (fixed)
Remove deprecated db parameters — at Version 27
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: |
|
||
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.
Change History (27)
comment:1 by , 10 years ago
comment:2 by , 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 , 10 years ago
Proposed changes in log:rjollos.git:t11605. The changes address the items raised in comment:1 and comment:2.
follow-ups: 5 6 comment:4 by , 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
.
comment:5 by , 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).
comment:6 by , 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 , 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.
follow-up: 11 comment:8 by , 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.
comment:9 by , 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?
comment:10 by , 10 years ago
Status: | new → assigned |
---|
comment:11 by , 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 easily 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): 231 231 cset = self.repos.get_changeset(next_youngest) 232 232 try: 233 233 # steps 1. and 2. 234 self. _insert_changeset(db,next_youngest, cset)234 self.insert_changeset(next_youngest, cset) 235 235 except Exception as e: # *another* 1.1. resync attempt won 236 236 self.log.warning('Revision %s already cached: %r', 237 237 next_youngest, e) … … class CachedRepository(Repository): 259 259 if feedback: 260 260 feedback(youngest) 261 261 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 262 269 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 """ 263 273 srev = self.db_rev(rev) 264 274 # 1. Attempt to resync the 'revision' table. In case of 265 275 # 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.
comment:12 by , 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.
comment:13 by , 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 , 10 years ago
API Changes: | modified (diff) |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
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 , 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 , 10 years ago
#11639 reported an issue with th:TimingAndEstimationPlugin that is due to removal of the deprecated parameters.
comment:19 by , 10 years ago
In [12856:12857], removed support for db
parameter from methods decorated with cached
.
follow-ups: 21 24 comment:20 by , 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:
comment:21 by , 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:23 by , 10 years ago
API Changes: | modified (diff) |
---|
comment:24 by , 10 years ago
API Changes: | modified (diff) |
---|
Replying to rjollos:
Eventually we may want to remove
Environment.get_db_cnx
,Environment.get_read_db
andEnvironment.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 , 10 years ago
API Changes: | modified (diff) |
---|
comment:26 by , 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:
- trunk/trac/ticket/report.py@12834:664#L649 - Just above the highlighted line.
- trunk/trac/versioncontrol/cache.py@13020:275#L269 - We'd need to extract the body of the
_insert_changeset
method to a third method, or somehow determine the caller is notinsert_changeset
. - trunk/trac/env.py@13064:717#L712 - Add an
else
. - trunk/trac/env.py@13064:754#L725 - Add an
else
.
The changes could probably wait until milestone:1.1.3.
comment:27 by , 10 years ago
API Changes: | modified (diff) |
---|
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/ $