Those needs to be cleaned up: T1532 (resulting from multiple team discussions).

I know @anlambert said otherwise but still, that task explains it all.
I never got a chance to finish this but i won't accept new code using that deprecated behavior.

You can however reference the needed configuration as comments.

that comes from self.config['temp_directory']

Please remove as it's unused.

Please stop using indirection, it's hard to follow through in plain text.
Be simple in your step, only use methods when you need to.

When the need to refactor into submethods arise, let's do it, not before.

Please, use some syntax checker (flycheck, flyspell, etc...)

Concealed this data into one dictionary to eliminate...
eliminate

Also, more generally, when in a submethod, please use the docstring to mention those.

self.package_details = {
    'name': name,
    'origin_url': origin_url,
    'tarballs': kwargs['tarballs'],
}

Thus really inline this in the caller method.

Make that a warn instead.
That's more critical, all the more reason since it triggers a partial visit.

By the way, i'm not sure we want to make that a partial visit [1]
But let's keep it that way for now.

[1] If an archive listed is 404, then we most probably never recover it.
So we'll stay in that partial visit forever.
On the other hand, having a full visit even though we missed some tarballs is not factually true as well...
so ¯\_(ツ)_/¯

I don't think we want to keep 'nature' key (which is not that great of a name, my bad).

IIRC, it's the tarball's type 'tar' or 'zip' here.

Remove that comment.

If it's really necessary it's not in the right place.

Prefer one return statement. Also a warn log statement when detecting the invalid tarball.

uncompressed_path = None
self.tarball_invalid = False

try:
  ...
  uncompressed_path = tarball.uncompress...
except:
  self.log.warn('Invalid tarball detected...')
  self.tarball_invalid

return path

Make that 'tar'.
We don't want to see 'zip' or something else here.

I'm not sure about this not being populated.
For information that's the revision parent list.
In dvcs, that usually reflects the commit/changeset history.

@olasd when you have a sec, what do you think?

That could be iterated over in a future iteration step, so i'm not blocking the diff for it

I'm thus curious about the snapshot here...

use 1 artifact release so 1 revision...

Because release is also part of the swh ontology, that's confusing ;)

Also, i'd keep the assertions to go in the right order:

• content
• directory
• revision
• release
• snapshot

Explicit the arguments, m is a mock.
Make it a better name... mock_<whatever-it-is>

m is not clear, please improve on its naming.

Tests can be seen as automated documentation, so it must be as readable (as good documentation do ;)

@anlambert i should have asked you also ^^

@ardumont, currently we do not populate the parents field in the PyPI and npm loader. We simply create a revision for each package version and reference them in a snapshot each time we visit the package.

Nevertheless, maybe it could be interesting to populate it with the revision pointing to the previous package version. This is semantically correct but as our data model is based on VCS ones I am not sure if we can do that. For instance, this could be useful to easily generate diffs between two successive package version in the web application.

I do not think that indirection is necessary at the moment. Simply explaining in the docstring that the generate_and_load_snapshot will gather package versions as branches and generate a snapshot from them is sufficient to understand what is is intended to do.

In the PyPI and npm loader, branches are named the following: releases/<package_version> (see here or here for instance).

We should keep that naming scheme for coherency. Here, this implies extracting the version number from the tarball name.

My bad for that as that snippet comes from the npm loader I wrote ;-)

Same here, I am the one to blame for the lazy naming.

s/coherency/consistency/

With gnu, it is not possible to get the package_version, so here I used the basename of url (as shown in docstring). Hope that is fine ;)

we are writing in temporary folder, make sure the tests are cleaning up after you run those.

I suppose this line will ensure this ;)

We use requests but i don't see mock for that, so i expect the tests to actually trigger http request, this needs to be mocked.

The tarball request is mocked here

This is a problem it should be temp_directory = self.config['temp_directory']

But while running it gives key error. Key temp_directory not found.

Hence I temporarily changed it to this.
How can I fix this, is there something I need to do in the loader?

When you're running the worker, is the temp_directory key set in the config? If not then you need to add it.