Page MenuHomeSoftware Heritage
Differential D1967

core/loader: Fix get_save_data_path implementation
ClosedPublic
Actions

Authored by ardumont on Sep 6 2019, 2:39 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDLDBASE78e735260ee7: package/loader: Fix mispelling
rDLDBASEfc191a0bd781: pytest.ini: Remove warnings about our custom markers
rDLDBASE161dc3906b61: pep8: Fix log.warning calls
rDLDBASE44cb763c7e93: core/loader: Fix get_save_data_path implementation
Summary

Related T1987

Also, unrelatedly:

  • pep8: Fix log.warning calls
  • pytest.ini: Remove warnings about our custom markers (see [1])
  • package/loader: Fix mispelling

[1] https://docs.pytest.org/en/latest/mark.html

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7710
Build 11076: tox-on-jenkinsJenkins
Build 11075: arc lint + arc unit

Event Timeline

ardumont created this revision.Sep 6 2019, 2:39 PM
swh-public-ci added a comment.Sep 6 2019, 2:40 PM

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

Harbormaster completed remote builds in B7710: Diff 6630.Sep 6 2019, 2:40 PM
vlorentz requested changes to this revision.Sep 6 2019, 2:43 PM

Needs a test.

There are also more commit than you probably intended in this diff

swh/loader/core/loader.py
214

is the isinstance actually needed? If yes: why is the url sometimes bytes and sometimes str?

This revision now requires changes to proceed.Sep 6 2019, 2:43 PM
ardumont added a comment.EditedSep 6 2019, 2:47 PM

There are also more commit than you probably intended in this diff

Well, yes and no but too lazy to do one commit, one diff.

I was annoyed by:

  • the bug
  • then the warnings
  • then thinking about all the shenanigans needed to actually land this...

I was and still am focused on debunking the loader-git in production right now.

swh/loader/core/loader.py
214

I'm not sure it is needed indeed.
It was to be on the safe side (since only the loader-git seems strike by this...)

I checked the storage's model, it's indeed a string so that should fairly be always a string.

ardumont updated this revision to Diff 6633.Sep 6 2019, 3:21 PM

Add missing test on method

It should have been posted along the commit that broke the behavior though ;)

(/me is a bad reviewer sometimes, see always room for improvments!)

swh-public-ci added a comment.Sep 6 2019, 3:23 PM

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

Harbormaster completed remote builds in B7713: Diff 6633.Sep 6 2019, 3:23 PM
vlorentz requested changes to this revision.Sep 6 2019, 3:24 PM
vlorentz added inline comments.
swh/loader/core/tests/test_loader.py
368–373 ↗(On Diff #6633)

Use a finally to remove it even on test failure. Or, even better: pytest's tmp_path fixture: http://doc.pytest.org/en/latest/tmpdir.html

This revision now requires changes to proceed.Sep 6 2019, 3:24 PM
ardumont added inline comments.Sep 6 2019, 3:25 PM
swh/loader/core/tests/test_loader.py
368–373 ↗(On Diff #6633)

oh yeah, i remember that now!

ardumont updated this revision to Diff 6634.Sep 6 2019, 3:31 PM

Use pytest fixture to deal with temporary dir

olasd added a subscriber: olasd.Sep 6 2019, 3:31 PM
olasd added inline comments.
swh/loader/core/tests/test_loader.py
365 ↗(On Diff #6633)

This will always be True. Double underscore attributes get a mangled name.

In [1]: class Foo:
   ...:     def __init__(self):
   ...:         self.__bla = None
   ...:         

In [2]: foo = Foo()

In [3]: foo.__dict__
Out[3]: {'_Foo__bla': None}

In [4]: hasattr(foo, '__bla')
Out[4]: False

I'm not sure what you're really asserting here?

vlorentz accepted this revision.Sep 6 2019, 3:32 PM
vlorentz added inline comments.
swh/loader/core/tests/test_loader.py
368 ↗(On Diff #6634)

this syntax is so weird...

This revision is now accepted and ready to land.Sep 6 2019, 3:32 PM
swh-public-ci added a comment.Sep 6 2019, 3:32 PM

Build has FAILED

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

Harbormaster failed remote builds in B7714: Diff 6634!Sep 6 2019, 3:32 PM
ardumont updated this revision to Diff 6635.Sep 6 2019, 3:35 PM

Remove useless assertion

swh/loader/core/tests/test_loader.py
368 ↗(On Diff #6634)

well, it wouldn't want me to mix the tuple and the PosixPath "string" so meh...

365 ↗(On Diff #6633)

Removed.

swh-public-ci added a comment.Sep 6 2019, 3:36 PM

Build has FAILED

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

Harbormaster failed remote builds in B7715: Diff 6635!Sep 6 2019, 3:36 PM
ardumont updated this revision to Diff 6639.Sep 6 2019, 6:06 PM
  • test_loader: Fix test?
swh-public-ci added a comment.Sep 6 2019, 6:07 PM

Build has FAILED

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

Harbormaster failed remote builds in B7719: Diff 6639!Sep 6 2019, 6:07 PM
ardumont updated this revision to Diff 6640.Sep 6 2019, 6:22 PM

os.path.join issue

swh-public-ci added a comment.Sep 6 2019, 6:23 PM

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

Harbormaster completed remote builds in B7720: Diff 6640.Sep 6 2019, 6:23 PM
ardumont updated this revision to Diff 6641.Sep 6 2019, 6:25 PM

Rebase and squash commits

swh-public-ci added a comment.Sep 6 2019, 6:26 PM

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

Harbormaster completed remote builds in B7721: Diff 6641.Sep 6 2019, 6:26 PM
Closed by commit rDLDBASE44cb763c7e93: core/loader: Fix get_save_data_path implementation (authored by ardumont). · Explain WhySep 6 2019, 6:26 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added inline comments.Sep 9 2019, 10:16 AM
swh/loader/core/tests/test_loader.py
70 ↗(On Diff #6642)

This is a private attribute. It won't be accessed by another other class and this one is empty.

douardda added a subscriber: douardda.Sep 9 2019, 10:29 AM
douardda added inline comments.
swh/loader/core/tests/test_loader.py
70 ↗(On Diff #6642)

yeah please, don't use 'private' attributes, like ever. This mangling magic stuff is nothing but garbage...

ardumont added inline comments.Sep 9 2019, 10:36 AM
swh/loader/core/tests/test_loader.py
70 ↗(On Diff #6642)

At the time, it was to make the test pass as this would not otherwise.
I'm currently trying to assert the removal somehow works now.

ardumont added inline comments.Sep 9 2019, 10:51 AM
swh/loader/core/tests/test_loader.py
70 ↗(On Diff #6642)

As it works without those, removed! I must have mis-analyzed the issue at the time.
Thanks for the heads up.

Revision Contents
Changeset List

PathSize
4 lines
swh/
loader/
core/
8 lines
package/
10 lines

Diff 6630

View Options

pytest.ini

Loading...
View Options

swh/loader/core/loader.py

Loading...
View Options

swh/loader/package/loader.py

Loading...
About · Developer information · Legal