Don't use @staticmethod in Crucible class

[spookylukey]

This hinders re-use by subclassing

[spookylukey]

See issue #3

[markdrago]

I made those methods @staticmethods because they didn't need access to anything in self. It just felt cleaner to me to avoid passing in parameters that aren't used. Would it not be better to just change the way the methods are being called to use self.methodname instead of Crucible.methodname? With that change the methods could stay static here and still be regular instance methods in subclasses. I'm not opposed to merging this, I just want to make sure that we're making the best change possible.

I'm also curious to know what you intend to do in a subclass. Is that subclass something that you plan on contributing as well?

[markdrago]

Oh right ... I have staticmethods calling staticmethods, which of course don't have access to self, so they have to say Crucible.methodname. Perhaps it would be better to just pull some of these functions out of the class entirely? Of course that would make it impossible to override their behavior in a subclass, but I don't know that it makes sense to change the behavior of some of these small helper functions. Again - I'm not opposed to merging this, but I'm curious to hear your thoughts on the alternatives I've suggested.

[spookylukey]

The ones that I've needed to override so far are:

1. get_create_review_payload
2. make_request

The first falls into the 'possible utility function' group you mentioned, which suggests it might be useful to be able to override them.

However, the reason I needed to override it was to make 'patch' optional, which I could do by passing a dummy patch in to the 'super' call and then changing the dictionary returned. (I'm creating a review without a patch and then adding changesets to it later, not a patch). If you prefer, I could provide a pull request for this instead (i.e. making 'patch' an optional argument to get_create_request_payload) - and also pull those 'payload' methods out into functions, if you think that's best.

[markdrago]

I think it would be best if you just added the support for making blank reviews to banter directly by adjusting the Crucible class itself. If you want to pull the static methods out of the class and just have them be free functions in the crucible module I think that makes sense, but it's up to you. It won't strictly be necessary if you aren't subclassing Crucible. Thanks!