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 7710
Build 11076: tox-on-jenkinsJenkins
Build 11075: 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
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

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.

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
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
swh/loader/core/tests/test_loader.py
368–373 ↗(On Diff #6633)

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
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 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

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.

Rebase and squash commits

This revision was automatically updated to reflect the committed changes.
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 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...

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.

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.