Implement pep440 blacklisting #16

dwighthubbard · 2018-05-16T22:33:58Z

This pull request has several related changes:

Implements a configuration class singleton to allow the configuration settings to be accessed where needed in code instead of passing the values as function/method arguments.
Adds Filter plugin classes for packages and releases.
Adds functions to query package_resources for the filtering plugin classes.
Moves the blacklist functionality to the BlacklistProject() plugin.
Updates the mirror object blacklist functionality to use the BlacklistProject() plugin.
Adds the new plugin entrypoints to the setup.py.
Adds tests for the plugin functionality.
Updated documentation for the new functionality.

to access the configuration.

- Add more documentation on filtering - Add stubs for the 2 types of filters to be implemented

…ep440_blacklisting # Manually fixed Conflicts: # src/bandersnatch/mirror.py # src/bandersnatch/package.py Fixed tests that broke due to update changes.

cooperlees

Long diff ! No wonder it took you all day.

Nice tests
Just some questions about introducing such a different style

Also, how do you feel about using the backport of importlib_resources for easy move in 3.7?

cooperlees · 2018-05-17T01:42:32Z

src/bandersnatch/configuration.py

+        if self.config_file:
+            config_files.append(self.config_file)
+        self.config = ConfigParser()
+        self.config.read(config_files)


Since this is a separate module, this already is a singleton ... So what does this "singleton" achieve that I am missing?

Using a singleton makes it easier to test since the module can be reinitialized with a different configuration during the tests.

cooperlees · 2018-05-17T01:47:42Z

src/bandersnatch/filter.py

+"""
+Blacklist management
+"""
+import pkg_resources


Should we look at using pip install importlib_resources with a easier move forward to the stdlib version in 3.7 ?

That would certainly be a good idea, but looking at the documentation for importlib_resources at https://docs.python.org/3.7/library/importlib.html#module-importlib.resources it looks like it implements only a subset of the functionality in pkg_resources and doesn't implement the functionality to iterate registered package entrypoints.

cooperlees · 2018-05-17T01:48:42Z

src/bandersnatch/filter.py

+from .configuration import BandersnatchConfig
+
+
+loaded_filter_plugins = defaultdict(lambda: [])


Just use defaultdict(list)

cooperlees · 2018-05-17T01:49:16Z

src/bandersnatch/filter.py

+loaded_filter_plugins = defaultdict(lambda: [])
+
+
+class Filter(object):


This is Python 3 - object is defaults

cooperlees · 2018-05-17T01:56:22Z

src/bandersnatch/package.py

-        ''' Take the JSON metadata we just fetched and save to disk '''
+        """
+        Take the JSON metadata we just fetched and save to disk
+        """


If these fit on 1 line < 80 chars what does this format achieve?

cooperlees · 2018-05-17T01:57:33Z

src/bandersnatch/tests/test__bandersnatch_plugins__blacklist_name.py

+    def setUp(self):
+        self.cwd = os.getcwd()
+        self.tempdir = TemporaryDirectory()
+        bandersnatch.filter.loaded_filter_plugins = defaultdict(lambda: [])


defaultdict(list)

cooperlees · 2018-05-17T01:58:22Z

src/bandersnatch/tests/test__bandersnatch_plugins__blacklist_name.py

+        if self.tempdir:
+            os.chdir(self.cwd)
+            self.tempdir.cleanup()
+            self.tempdir = None


What does setting tmpdir to None do?

It's defensive coding since the teardown only removes the tempdir if one was created and stored to the self.tempdir attribute.

I think cleanup() does that.

cooperlees · 2018-05-17T02:00:29Z

src/bandersnatch/tests/test__bandersnatch_plugins__blacklist_name.py

+    def setUp(self):
+        self.cwd = os.getcwd()
+        self.tempdir = TemporaryDirectory()
+        bandersnatch.filter.loaded_filter_plugins = defaultdict(lambda: [])


What does lambda achieve?

cooperlees · 2018-05-17T02:02:26Z

src/bandersnatch/tests/test_mirror.py

-    m.packages_to_sync = {'example1': None, 'example2': None}
-    m._remove_blacklisted_packages()
-    assert blacklisted_package not in m.packages_to_sync
+def test_mirror__filter_packages__match(tmpdir):


Why double __ ? What does it mean?

It is to differentiate between spaces and underscores since the function names being tested have underscores in their name and I like the test to include the name of the function being tested.

Is this a standard or something? Never heard of it.

cooperlees · 2018-05-17T02:03:49Z

tox.ini

@@ -3,7 +3,7 @@ envlist = py36

 [testenv]
 deps = -rtest-requirements.txt
-commands = pytest
+commands = pytest -vv


Sometimes I find this to much output ... But I guess it does not hurt

…ep440_blacklisting

…nto implement_pep440_blacklisting

Remove -vv from pytest run.

cooperlees

Thanks for the feature.

In parts it seems a little complicated and I'm not sold on the entry points but we can always fix it if it becomes a problems.

cooperlees · 2018-05-18T09:12:15Z

src/bandersnatch/configuration.py

+from configparser import ConfigParser
+
+
+class Singleton(type):  # pragma: no cover


I'll look it up, but no idea why we need type here.

cooperlees · 2018-05-18T09:13:20Z

src/bandersnatch/tests/test__bandersnatch_plugins__blacklist_name.py

+        if self.tempdir:
+            os.chdir(self.cwd)
+            self.tempdir.cleanup()
+            self.tempdir = None


I think cleanup() does that.

cooperlees · 2018-05-18T09:14:10Z

src/bandersnatch/tests/test_mirror.py

-    m.packages_to_sync = {'example1': None, 'example2': None}
-    m._remove_blacklisted_packages()
-    assert blacklisted_package not in m.packages_to_sync
+def test_mirror__filter_packages__match(tmpdir):


Is this a standard or something? Never heard of it.

dhubbard added 7 commits May 15, 2018 14:32

Add a Singleton to hold configuration that can be used where things need

a472fbb

to access the configuration.

Add a missing testcase to ensure the singleton propagates to instances.

184e471

Merge changes since move to PYPA

8922592

- Add more documentation on filtering - Add stubs for the 2 types of filters to be implemented

Get filters for new build in blacklist plugins

04bbfe5

Add code to implement release filters

05a098c

Merge remote-tracking branch 'remotes/origin/master' into implement_p…

0501b06

…ep440_blacklisting # Manually fixed Conflicts: # src/bandersnatch/mirror.py # src/bandersnatch/package.py Fixed tests that broke due to update changes.

Remove unused variable from filter.py

4a152bd

cooperlees requested changes May 17, 2018

View reviewed changes

cooperlees added the enhancement New feature or request label May 17, 2018

dhubbard and others added 6 commits May 17, 2018 07:00

Merge remote-tracking branch 'remotes/origin/master' into implement_p…

bd5956e

…ep440_blacklisting

Merge branch 'master' into implement_pep440_blacklisting

2005204

Merge remote-tracking branch 'origin/implement_pep440_blacklisting' i…

18e718d

…nto implement_pep440_blacklisting

Remove the lambda from the defaultdict calls

909f03a

Remove -vv from pytest run.

Cleanup some documentation issues.

c2a4436

Fix another documentation issue

d886cb5

cooperlees approved these changes May 18, 2018

View reviewed changes

cooperlees merged commit 945dece into master May 18, 2018

cooperlees deleted the implement_pep440_blacklisting branch May 18, 2018 09:15

dwighthubbard mentioned this pull request May 21, 2018

Update the blacklist filtering to allow specifying project and version #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement pep440 blacklisting #16

Implement pep440 blacklisting #16

dwighthubbard commented May 16, 2018

cooperlees left a comment

cooperlees May 17, 2018

dwighthubbard May 17, 2018

cooperlees May 17, 2018

dwighthubbard May 17, 2018

cooperlees May 17, 2018

cooperlees May 17, 2018

cooperlees May 17, 2018

cooperlees May 17, 2018

cooperlees May 17, 2018

dwighthubbard May 17, 2018

cooperlees May 18, 2018

cooperlees May 17, 2018

cooperlees May 17, 2018

dwighthubbard May 17, 2018

cooperlees May 18, 2018

cooperlees May 17, 2018

cooperlees left a comment

cooperlees May 18, 2018

cooperlees May 18, 2018

cooperlees May 18, 2018

		from .configuration import BandersnatchConfig


		loaded_filter_plugins = defaultdict(lambda: [])

		loaded_filter_plugins = defaultdict(lambda: [])


		class Filter(object):

		from configparser import ConfigParser


		class Singleton(type): # pragma: no cover

Implement pep440 blacklisting #16

Implement pep440 blacklisting #16

Conversation

dwighthubbard commented May 16, 2018

cooperlees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cooperlees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment