Should this be "File based…"?

You can use urlparse to get these components split out nicely:

https://docs.python.org/2/library/urlparse.html

You can also use urljoin to construct these URLs:

https://docs.python.org/2/library/urlparse.html#urlparse.urljoin

If you want to ignore the value, all you need to do is not assign it to anything.

I recommend using ticket trackers for TODOs instead of putting them in the code.

Why assign columns if you are going to raise an Exception?

I recommend adding a comment here explaining why/when table_def would not be a dictionary.

I recommend adding a docblock here to document what each of these is for, and what type it is. It's not clear below what table_def['columns'] is expected to be, for example, so it's hard for me to follow this code.

I recommend a docblock here too.

I recommend documenting the parameters, types of parameters, and return value and return type here.

Modifying an iterable as you iterate can be risky. I recommend against doing it this way. A better way would be to keep track of which keys you wanted to delete in a list and doing the deletes after the iteration.

I recommend adding docblocks to both of the above, explaining what they are for and why we don't want them to do anything here (or at least the latter).

Docblock.

Why do we want to pass on this Exception? I'd recommend documenting that here for future readers.

Docblock.

Docblock.

IMO this function is a bit long and hard to follow as a result. I'd recommend making some subfunctions to break the work up into smaller logical blocks. This makes the code easier to follow, but it also makes testing easier since you can test the units by themselves instead of having to test them together.

Docblock.

Docblock.

Docblock.

You could use urlparse here too, if you want.

I recommend adding docblocks on all of the above.

I recommend docblocks on every unit in this class, including the class itself.

If you use unittest with individual tests, you don't have to sys.exit() whenever you encounter an error. I think this is valuable because if there is more than one test failure you can know which tests fail in a single run, rather than having it exit upon hitting the first failure. By the way, unittest test runners often have a mode that can exit upon hitting the first failure too, so that way you can still have this behavior when wanted (best of both worlds!) For example, nosetests -x will exit upon the first failure, but plain nosetests will run them all and then tell you which ones failed.

Nothing in this seems like a no-go, but I do have some recommendations for you. In particular, I'd like to call attention to that loop that is deleting items as it iterates. That could lead to a bug. I also highly recommend adding docblocks on everything and explaining the answers to my questions in code comments so they stay answered for future readers ☺

That's why I'm not doing that: I'm iterating the self._primary_key list, and I'm only doing an "if pkey in kvfilter" on every iteration.

This object is only supposed to be instantiated by the Store classes. For the definitions of table_def's, they are in data.py at the top: UNIQUE_DATA_TABLE and OPTIONS_TABLE.

This line appears to be identical to line 27 - you could pull both out to happen before the if statement to deduplicate code.

It might be good to rename config to uri or something else that's more descriptive.

You could turn this into a docblock for this method, and expand on it a bit.

You could raise ValueError here as a more specific Exception, since it was the values of the arguments that caused the Exception.

It would be good to document all of these arguments and their types in a docblock, and mention the raising of the exception.

I recommend adding test coverage here so you can ensure this works as you expect.

Why do you want to break when you hit a key that isn't in the kvfilter? I recommend adding a comment explaining that here.

I think you can drop the call to list() here, as the list comprehension is already a list.

I recommend adding a docblock, and documenting the arguments and their types.

I recommend a docblock here.

docblock

This seems like a common pattern. Should the Exception be added to _get_most_specific_driver() so this isn't repeated? If not, consider making a helper wrapper method that does this if/raise pattern for you so you don't have to repeat yourself.

Additionally, consider using a ValueError here instead of the generic Exception.

This could be a ValueError as well.

docblock.

Adding a comment here explaining why we want to return here would be handy.

docblock

docblock

docblock

docblock.

docblock

docblock

2017

LGTM, everything I wrote is just suggestions. Sorry about the e-mailed one you probably got about deleting a key while iterating - I had misread the code. I deleted that comment here so it won't appear.

I am fully aware of that, but that's a whole project in itself.
We are currently using a mostly manual test framework because of test inter-dependencies and the fact that our test case is not your average one.
For one, we don't run the "system under test" inside our own process: we start an apache webserver to actually run it.

This is used quite heavily by the test suite in general.

Looks good.

This has been merged as d1521504f2ee1688b3c1eacf1458ec9c2064ccd5, e352e726f7359226b8f2da8d3c34483a6872d2c5 and 008b1e0eec89513cfa8269d3d56f7e723bf7b5d6 .