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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7715
Build 11085: tox-on-jenkinsJenkins
Build 11084: arc lint + arc unit

Event Timeline

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
213

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

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
213

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.

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
366–371

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
swh/loader/core/tests/test_loader.py
366–371

oh yeah, i remember that now!

Use pytest fixture to deal with temporary dir

olasd added inline comments.
swh/loader/core/tests/test_loader.py
363

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 added inline comments.
swh/loader/core/tests/test_loader.py
367

this syntax is so weird...

This revision is now accepted and ready to land.Sep 6 2019, 3:32 PM

Remove useless assertion

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

Removed.

367

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

Rebase and squash commits

This revision was automatically updated to reflect the committed changes.
swh/loader/core/tests/test_loader.py
69

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
69

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

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

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.

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

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