Differential D2586

Make db_transaction* remove db/cur from the signature.
ClosedPublic
Actions

Authored by vlorentz on Jan 24 2020, 5:45 PM.

Details

Reviewers

ardumont

Group Reviewers

Reviewers

Commits

rDCORE32423fb1d5a6: Make db_transaction* remove db/cur from the signature.

Summary

Rremoving them allows testing the function's signature
matches the existing signature of a specification and type checking.

Moreover, they should not be used by users of the class, so there is no
reason to have them appear in the documentation (generated from
the signature).

Depends on D2585.

Diff Detail

Repository

rDCORE Foundations and core functionalities

Branch

db_transaction_signature

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 10282
Build 15266: tox-on-jenkins	Jenkins
Build 15265: arc lint + arc unit

Event Timeline

vlorentz created this revision.Jan 24 2020, 5:45 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJan 24 2020, 5:45 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DCORE/job/tox/393/
See console output for more information: https://jenkins.softwareheritage.org/job/DCORE/job/tox/393/console

Harbormaster failed remote builds in B10282: Diff 9221!Jan 24 2020, 5:47 PM

vlorentz added a child revision: D2587: Move Storage documentation and endpoint paths to a new StorageInterface class.Jan 24 2020, 5:53 PM

I agree with the reasoning in the description.

As it's in core though, I'm unsure about the impacts so if someone else could review as well, that'd be neat.

Note that i'm wondering whether it would not be best to constraint this parameters removal to the storage test defined in your other diff
As far as i could tell, this is for that unique test [1].

[1] https://forge.softwareheritage.org/D2587#inline-17172

swh/core/db/common.py
11	docstring

This revision is now accepted and ready to land.Jan 27 2020, 11:11 AM

ardumont mentioned this in D2585: Add tests for db_transaction and db_transaction_generator..Jan 27 2020, 11:12 AM

In D2586#61408, @ardumont wrote:

As it's in core though, I'm unsure about the impacts so if someone else could review as well, that'd be neat.

Function signatures have no impact on code execution. It only affects code using introspection; which for us is only sphinx.

rebase

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/397/ for more details.

Harbormaster completed remote builds in B10295: Diff 9234.Jan 27 2020, 1:38 PM

fix wrapped function name

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/398/ for more details.

Harbormaster completed remote builds in B10322: Diff 9262.Jan 28 2020, 2:09 PM

Closed by commit rDCORE32423fb1d5a6: Make db_transaction* remove db/cur from the signature. (authored by vlorentz). · Explain WhyJan 29 2020, 12:20 PM

This revision was automatically updated to reflect the committed changes.

vlorentz added a commit: rDCORE32423fb1d5a6: Make db_transaction* remove db/cur from the signature..

As it's in core though, I'm unsure about the impacts so if someone else could review as well, that'd be neat.

Function signatures have no impact on code execution. It only affects code using introspection; which for us is only sphinx.

https://jenkins.softwareheritage.org/job/DCIDX/job/tests/816/console build disagrees.
As far as i could tell, as we are piling other decorators, that somehow makes it fail.
Removing the remove_kwargs decorator (or avoiding messing with f.signature) and everythings get back to its feet.

Yes, apparently, Flask also inspects signatures.

I'll fix the issue in swh-indexer

Revision Contents
Changeset List

Path

Size

swh/

core/

db/

common.py

17 lines

tests/

test_db.py

31 lines

Diff 9221

View Options

swh/core/db/common.py