I tend to use os.path.basename even on url, that works, any counter indication to that?

Maybe log that we did not find it?

Not blocking but as a heads up, we tend towards moving away from unittest now and try to embrace the pytest and its fixture ;)

What that entails is we can drop the class thing and use function directly, letting pytest injects its or even our own fixture dependencies (tmpdir, test data, client, etc...) as function parameter.

(Again, not blocking ;)

We could even demo that it's supposed to be the same snapshot (using the id is enough) as the first round.

As in (from previous test):

self.assertSnapshotEqual(_expected_new_snapshot_first_visit,
                         _expected_branches_first_visit)

Why 2 snapshots?

I do use urllib.parse.urlparse() for this kind of stuff. In this case, it helps managing params and query elements one can find in an URL.

Not sure if we can encounter real urls as tarball_url here, but I would rather use urllib stuff by default.

Right!

I would go for the os.path.basename approach as it induces less instructions to extract the filename, see below:

>>> urllib.parse.urlparse('https://registry.npmjs.org/webpack/-/webpack-0.1.2.tgz')
ParseResult(scheme='https', netloc='registry.npmjs.org', path='/webpack/-/webpack-0.1.2.tgz', params='', query='', fragment='')
>>> os.path.basename('https://registry.npmjs.org/webpack/-/webpack-0.1.2.tgz')
'webpack-0.1.2.tgz'

ack

I need to document myself about pytest fixtures first. I keep it like this at the moment.

Indeed, I should have added that test here.

Because a single instance of the npm loader is used in these tests. So after the second visit, two snapshots will be stored in the in-memory storage. I proceeded like this as I wanted to test that only new found objects are sent to the storage after the first visit. If I instantiated a loader before each test, the in-memory storage is recreated from scratch and I did not want that to happen.

Sure thing!

ack.

You should document that this class cannot be used to manipulate multiple packages at once.

known_versions should be renamed skip_versions, so it's clearer what it does.

Type could be Union[dict, set], as this function uses only features available for both

Same comment as above

You should use a namedtuple instead.

You don't need safe='', the way you parse package names prevents them from containing slashes.

I think you can use .encode('ascii') here. From npm's specs:

The name ends up being part of a URL, an argument on the command line, and a folder name. Therefore, the name can’t contain any non-URL-safe characters.

Same:

Version must be parseable by node-semver

Same

No need for safe=''.

No need to inherit from object explicitely

ack

as said orally by @ardumont, let's keep the parameter names close to those that can be founded in the PyPI loader implementation. It will ease future refactoring.

Unfortunately, package names containing '/' exist, for instance https://www.npmjs.com/package/@01ht/ht-date, so that's why I need to override the safe parameter.

As you can see below:

https://replicate.npmjs.com/@01ht/ht-date => package not found
https://replicate.npmjs.com/%4001ht%2Fht-date => package found

Considering that dir_path is a concatenation of a temporary directory and the package name, I would keep the utf-8 decoding here. Indeed, the temporary directory can be provided by configuration so it may contain some non ascii characters in it.

ack

ack

will simplify for test

ack