How are you running the tests?

~/Downloads/Code/reviewboard $ ./reviewboard/manage.py test

Well, that should be correct.
Probably what we need to do is try/catch on the imports so that the file itself can be imported regardless of whether the backend class is actually usable or not. The current pysvn implementation does this, as do other SCMTools like bazaar.
We should probably have a discussion about whether or not we want to retain pysvn compatibility or just switch over to subvertpy.

Probably what we need to do is try/catch on the imports so that the file itself can be imported regardless of whether the backend class is actually usable or not. The current pysvn implementation does this, as do other SCMTools like bazaar.

OK, now those try-except blocks make more sense. I will make those changes and submit an updated diff shortly.



We should probably have a discussion about whether or not we want to retain pysvn compatibility or just switch over to subvertpy.

Apart from backwards compatibility, the reason why I kept PySVN support in Review Board is because subvertpy has not yet released a Python 3-compatible version yet, although there has been a fair amount of work done to that end.

If Subvertpy was a pure Python module, I'd say let's nuke PySVN and just depend on it. Since it still has to be compiled, and likely isn't packaged on many distros, I think having both implementations (and documenting just subvertpy for now) is the best way to go for the time-being, since it's far less work for those upgrading, and less likely to break working installations.

subvertpy is actually packaged on most major distros that I know of, except RHEL/CentOS and related. If you want me to change the documentation to only mention subvertpy (right now my diff has both), that's easy enough, compared to fixing this import test...

We still need the import test fixed, either way.
I think the docs should encourage Subvertpy, but mention PySVN as a fallback. It's very possible we'll hit issues with Subvertpy that we're not aware of yet.
RHEL/CentOS are pretty important, but I imagine once we have this support, they can get Subvertpy packaged fairly easily if they choose. I'm more concerned about older distros (not everyone upgrades their servers very regularly). I haven't looked into it though.

We still need the import test fixed, either way.

Sorry, I didn't mean to imply that I wasn't going to do that, it's just a little bit more involved than I had hoped.



I think the docs should encourage Subvertpy, but mention PySVN as a fallback. It's very possible we'll hit issues with Subvertpy that we're not aware of yet.

OK, I'll work on changing the docs to reflect this.



RHEL/CentOS are pretty important, but I imagine once we have this support, they can get Subvertpy packaged fairly easily if they choose. I'm more concerned about older distros (not everyone upgrades their servers very regularly). I haven't looked into it though.

When I made my statement about RHEL/CentOS, I was going off of the current state of the Review Board docs that say that PySVN isn't packaged in RHEL/CentOS as well. I just did a quick search, and both PySVN and subvertpy are not in the core RHEL repositories, but they are in the EPEL repository. I can update the docs to reflect this as well.

The # NOQA is for flake8. I'll change it to:

# flake8: NOQA

We don't use flake8. I' prefer not to litter the code with comments for linters.

(Didn't mean to make a separate review for this comment...)
It's the equivalent code from the original function. It creates the client and makes sure that the SSL cert is valid.

I forgot to remove this comment before publishing. I had figured it out after reading through the code more.