[PATCH] example cred checker for pypgsql/adbapi.

[matiu]

I needed dbcred so I wrote the attached file. If someone could please put it in
twisted that it be cool. I've tested it with the default settings. I'm using it
in my production code. Probably needs more testing though.

Thanks :)

[matiu]

The contents of the file are:
 A database checker object that implements IChecker

[itamarst]

Shouldn't be tied to pyPgSQL I don't think, and ought to include sample SQL
schema... I'm not sure this should be in Twisted proper, really. Might make a
good example though, and certainly posting this to the list would be useful to
other people.

[davep]

This would be a good example. As part of putting it in the examples,
I think we can remove the deprecated code in enterprise.

[therve]

Let's save dave :).

I've updated the patch to work with current Twisted, and a example to run it (not really clean :/).

[davep]

Thanks :) I swear to take a look at it this weekend.

[exarkun]

Something about this patch bothers me. I guess it is the large amount of fairly non-trivial code with no test coverage.

[therve]

Replying to exarkun:

Something about this patch bothers me. I guess it is the large amount of fairly non-trivial code with no test coverage.

I guess you're talking about the ssh server initialization ? The example itself is pretty simple, but the main function is really dirty. I wanted to add a ssh server because I wanted a easily testable server, but I guess something with pb will be easier.

[davep]

Thanks for submitting this, therve, you put me to shame :)

Regarding exarkun's, comment, this does look like it would become the largest example in the docs directory, at least in terms of line count. Though some of the length is due to documentation that a lot of the existing examples don't have.

I do think it could be significantly shortened, though. Using PB might be simpler than conch. Also, a lot of the options, like 'caseSensitiveUsernames', don't really contribute to understanding -- anyone could add those once they understood the essentials.

What do you think?

[therve]

In fact I'd just tak the sql query as parameter, that would prevent all these options. Also I could switch to PB to reduce the main.

[davep]

Replying to therve:

In fact I'd just tak the sql query as parameter, that would prevent all these options. Also I could switch to PB to reduce the main.

Yep, that sounds good.

[therve]

OK the main section is now much simpler, and with using pbecho I'm now compatible with pbechoclient, which is good.

[davep]

This looks good to me, though I think 'caseSensitivePasswords' could probably be left out as well. OTOH, showing a little variation in the auth code might be helpful. Anyone object to putting in this example as is?

[exarkun]

A few minor points:

1. The file should have a copyright statement.
2. There are some simple suites in the code, these should be expanded.
3. The docstring for init talks about PyPgSQL, but the code actually uses PySQLite2. Having two different query strings and only using one makes the example more complex than it needs to be. Likewise with the customCheckFunc, which is not passed in by the driver code. The example should probably focus on one thing and do really well at conveying the details relevant to that thing. Hashed password checking, case insensitivity, and cross-database compatibility could probably each use an independent example in addition to this one.
4. The module docstring could probably do with some prose giving the overall point of the example.
5. There's a local variable db_deferred which should be dbDeferred to conform to the naming convention.

Of these, 3 probably isn't a blocker, and the rest are trivial. This is certainly an improvement over the existing documentation situation, but I think it could be even better if we really focus on the concepts we're trying to communicate.

[therve]

I did the modifications of 1,2,4,5 and some of point 3. I'm reluctant to remove code from the example, as it's a pretty good full-featured example, and I don't think it makes it so complex. We can also provide different main methods to run different examples ?

But I'm not completely insensible to your argument, so if I didn't convince you I'll simplify it :).

[therve]

I forget, the file is now in branch db-cred-460.

[exarkun]

I'm not unconvinced, but I'm not completely convinced either. Most of my reservations are based on problems which every other example we have shares though, so I'm will to let this go until we have a general solution we can apply to everything. The problem is just that this is code with no test coverage, so it's probably broken. ;)

Thanks for doing the other cleanups. Just one more thing: I'm not sure we should suggest using the code as-is in the module docstring. It's an example, it's good for helping users get an idea of how this kind of thing works. If they also decide they can copy it into their project and use it, that's their problem. ;) I'd rather not suggest we do this, though (until we have unit tests we can suggest they copy as well, at least).

[therve]

(In [19793]) Merge db-cred-460

Author: matiu, therve Reviewer: exarkun, davep Fixes #460

Add an example cred checker using adbapi.

[therve]

Until we find a solution (I'm not sure about unittests... maybe running the examples directly?), we could add a step to the review process 'Try to run the examples that may be changed by the branch'. It's not that great, but it's already a step toward better examples.