Page MenuHomeSoftware Heritage

core/loader: Fix get_save_data_path implementation
ClosedPublic

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

Details

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 Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Fri, Sep 6, 2:39 PM
vlorentz requested changes to this revision.Fri, Sep 6, 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.Fri, Sep 6, 2:43 PM
ardumont added a comment.EditedFri, Sep 6, 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.Fri, Sep 6, 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!)

vlorentz requested changes to this revision.Fri, Sep 6, 3:24 PM
vlorentz added inline comments.
swh/loader/core/tests/test_loader.py
367–372

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.Fri, Sep 6, 3:24 PM
ardumont added inline comments.Fri, Sep 6, 3:25 PM
swh/loader/core/tests/test_loader.py
367–372

oh yeah, i remember that now!

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

Use pytest fixture to deal with temporary dir

olasd added a subscriber: olasd.Fri, Sep 6, 3:31 PM
olasd added inline comments.
swh/loader/core/tests/test_loader.py
364

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.Fri, Sep 6, 3:32 PM
vlorentz added inline comments.
swh/loader/core/tests/test_loader.py
368

this syntax is so weird...

This revision is now accepted and ready to land.Fri, Sep 6, 3:32 PM
ardumont updated this revision to Diff 6635.Fri, Sep 6, 3:35 PM

Remove useless assertion

swh/loader/core/tests/test_loader.py
364

Removed.

368

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

ardumont updated this revision to Diff 6639.Fri, Sep 6, 6:06 PM
  • test_loader: Fix test?
ardumont updated this revision to Diff 6640.Fri, Sep 6, 6:22 PM

os.path.join issue

ardumont updated this revision to Diff 6641.Fri, Sep 6, 6:25 PM

Rebase and squash commits

This revision was automatically updated to reflect the committed changes.
vlorentz added inline comments.Mon, Sep 9, 10:16 AM
swh/loader/core/tests/test_loader.py
70

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

douardda added inline comments.
swh/loader/core/tests/test_loader.py
70

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

ardumont added inline comments.Mon, Sep 9, 10:36 AM
swh/loader/core/tests/test_loader.py
70

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.Mon, Sep 9, 10:51 AM
swh/loader/core/tests/test_loader.py
70

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