Page MenuHomeSoftware Heritage

core/loader: Fix get_save_data_path implementation
ClosedPublic

Authored by ardumont on Sep 6 2019, 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/Package 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.Sep 6 2019, 2:39 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!)

vlorentz requested changes to this revision.Sep 6 2019, 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.Sep 6 2019, 3:24 PM
ardumont added inline comments.Sep 6 2019, 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.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
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.Sep 6 2019, 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.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
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.Sep 6 2019, 6:06 PM
  • test_loader: Fix test?
ardumont updated this revision to Diff 6640.Sep 6 2019, 6:22 PM

os.path.join issue

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

Rebase and squash commits

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

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.Sep 9 2019, 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.Sep 9 2019, 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.