Page MenuHomeSoftware Heritage
Differential D4431

Refactor BaseApiDepositClient to get rid of the _client argument
ClosedPublic
Actions

Authored by douardda on Nov 6 2020, 11:15 AM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDDEP1129a092e671: Refactor BaseApiDepositClient to get rid of the _client argument
Summary

This argument was only there to make it easier to mock the HTTP calls
(via a mocked requests). Thanks to the mockup capabilities, this is in
fact not necessary. So get rid of it and replace its usage in tests by
proper use of the mocker pytest fixture.

Also:

  • refactor this BaseApiDepositClient constructor to acces directly "url" and "auth" kwargs instead of an unspecified generic config dict (still supported but deprecated),
  • get rid of a few actually not really needed fixtures (deposit_config)
  • rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest functions,
  • get rid of onliners functions in swh.deposit.cli.client (_client(), deposit_create, deposit_update).

Diff Detail

Repository
rDDEP Push deposit
Branch
refactorings
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 16991
Build 26220: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 26219: arc lint + arc unit

Event Timeline

douardda created this revision.Nov 6 2020, 11:15 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptNov 6 2020, 11:15 AM
douardda added a child revision: D4432: Simplify `swh deposit upload` cli options.Nov 6 2020, 11:15 AM
swh-public-ci added a comment.Nov 6 2020, 11:18 AM

Build is green

Patch application report for D4431 (id=15687)

Rebasing onto 0f4ec31168...

Current branch diff-target is up to date.
Changes applied before test
commit 77900028c2c694fc67b1c3fa1046de60aa5e80f5
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/285/ for more details.

Harbormaster completed remote builds in B16901: Diff 15687.Nov 6 2020, 11:18 AM
vlorentz requested changes to this revision.Nov 6 2020, 12:24 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/deposit/client.py
134

it doesn't just notify, but also computes a value.

162–166

these four lines look a lot like the code in notify_deprecated.

(also, the or None is redundant)

172

move the comment on the line before the assert, so black doesn't need to add newlines like this

swh/deposit/tests/loader/test_client.py
37–38

not covered?

also, "passzord"

This revision now requires changes to proceed.Nov 6 2020, 12:24 PM
douardda updated this revision to Diff 15726.Nov 9 2020, 9:43 AM

rebase

swh-public-ci added a comment.Nov 9 2020, 9:46 AM

Build is green

Patch application report for D4431 (id=15726)

Rebasing onto 7148a257b2...

Current branch diff-target is up to date.
Changes applied before test
commit 5e05355a83ae33a03d2daa60052dda3c600ac9c2
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/295/ for more details.

Harbormaster completed remote builds in B16941: Diff 15726.Nov 9 2020, 9:46 AM
douardda marked 3 inline comments as done.Nov 9 2020, 9:53 AM
douardda updated this revision to Diff 15730.Nov 9 2020, 9:54 AM

Fixes according to vlorentz' comments

ardumont added a subscriber: ardumont.Nov 9 2020, 9:55 AM
ardumont added inline comments.
swh/deposit/cli/client.py
249

It's already there line 254 below.

swh-public-ci added a comment.Nov 9 2020, 9:57 AM

Build is green

Patch application report for D4431 (id=15730)

Rebasing onto 7148a257b2...

Current branch diff-target is up to date.
Changes applied before test
commit bda0d3b5d7e33fd9e981f075a8f3ac425698d442
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/298/ for more details.

Harbormaster completed remote builds in B16945: Diff 15730.Nov 9 2020, 9:57 AM
douardda added inline comments.Nov 9 2020, 10:01 AM
swh/deposit/tests/loader/test_client.py
37–38

not sure I get the "not covered?" part here

ardumont added inline comments.Nov 9 2020, 10:26 AM
swh/deposit/tests/loader/test_client.py
37–38

the jenkins coverage report on the right (once jenkins build is done) for the else block is orange (so it's not covered).

douardda added inline comments.Nov 9 2020, 10:42 AM
swh/deposit/cli/client.py
249

oops, thx

douardda updated this revision to Diff 15735.Nov 9 2020, 10:45 AM

fix a few more oopsies reported by vlorentz and ardumont

swh-public-ci added a comment.Nov 9 2020, 10:48 AM

Build is green

Patch application report for D4431 (id=15735)

Rebasing onto 7148a257b2...

Current branch diff-target is up to date.
Changes applied before test
commit 7e69a87f813630102696a2a0a4238cf5cf7545ff
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/299/ for more details.

Harbormaster completed remote builds in B16949: Diff 15735.Nov 9 2020, 10:48 AM
vlorentz accepted this revision.Nov 9 2020, 4:17 PM
This revision is now accepted and ready to land.Nov 9 2020, 4:17 PM
douardda updated this revision to Diff 15780.Nov 10 2020, 2:46 PM

rebase

swh-public-ci added a comment.Nov 10 2020, 2:51 PM

Build is green

Patch application report for D4431 (id=15780)

Rebasing onto 018500c195...

Current branch diff-target is up to date.
Changes applied before test
commit c7e397e527a13fc09ffb314c3a2cc9df69712160
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/313/ for more details.

Harbormaster completed remote builds in B16991: Diff 15780.Nov 10 2020, 2:51 PM
douardda updated this revision to Diff 15801.Nov 10 2020, 5:30 PM

rebase

swh-public-ci added a comment.Nov 10 2020, 5:33 PM

Build is green

Patch application report for D4431 (id=15801)

Rebasing onto c815bbf19a...

Current branch diff-target is up to date.
Changes applied before test
commit 1129a092e6718c1c7bfc8bd467d4f389066f6b65
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/318/ for more details.

Harbormaster completed remote builds in B17011: Diff 15801.Nov 10 2020, 5:33 PM
Closed by commit rDDEP1129a092e671: Refactor BaseApiDepositClient to get rid of the _client argument (authored by douardda). · Explain WhyNov 10 2020, 6:42 PM
This revision was automatically updated to reflect the committed changes.
douardda added a commit: rDDEP1129a092e671: Refactor BaseApiDepositClient to get rid of the _client argument.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
deposit/
cli/
85 lines90%
Loading...
107 lines--
tests/
cli/
22 lines100%
Loading...
loader/
95 lines100%
Loading...

Diff 15780

View Options

swh/deposit/cli/client.py

Loading...
View Options

swh/deposit/client.py

Loading...
View Options

swh/deposit/tests/cli/test_client.py

Loading...
View Options

swh/deposit/tests/loader/test_client.py

Loading...
About · Developer information · Legal