Add channel as constructor's parameter and to snapshot function

[coveralls]

Coverage decreased (-0.2%) to 31.56% when pulling 2ba077d15a66e4d543c9d10797c685b0ddbf5adc on akram:add-channel-as-class-attribute into 6db2d9cac09df0414b8ad29c04c276368e348210 on tchellomello:master.

[pnbruckner]

If you're not ready for a review and are still working on this, that's fine, but please change the title to "WIP: ..."

[akram]

Hi @pnbruckner ,

I know that it is not used now. My primary goal is to provide a patch for the home-assistant amcrest integration. And exposing this parameter is a path to do it. It is now unused because I don't want to break compatibility or change other methods signatures in a useless manner or to change bevahior.

As an example, the snapshot() function: The current implementation already takes channel as a parameter. Having now channel as a class attribute would change the default implementation which I don't want to do to avoid long reviews. Especially, that there is poor test coverage right now.

What can I do to move forward ?

[pnbruckner]

@akram adding a class attribute whose default value is None (if a value is not passed in through the constructor), and has no effect when it is None, would not be a breaking change. Adding keyword parameters to existing methods, also with a default value of None, which has no effect when None, would also not be a breaking change.

I would suggest, when you have something that uses the new class attribute (when it's not None), and provides useful new functionality, then that would be the time to submit a PR. I'd also suggest getting it working in your own fork first, before submitting the PR. As it is, every time you did a force push I (and I'm assuming the repo owners) got notifications.

But that's just my opinion. I don't own this repo. The owner was just kind enough to grant me write privileges because I was making a lot of improvements.

[dougsland]

@flacjacket Is it something you introduce https://github.com/tchellomello/python-amcrest/pull/169 ?

[flacjacket]

Right, this is similar to what was introduced in #174, but the ability to do snapshots on specific channels was added earlier, including being fixed up in d5dbef9. The #174 changes make it so all of the other functionality that would use channels is also able to do so, but by using method parameters, rather than adding the channel to the top-level AmcrestCamera object. I think using the per-method channel is better than using a per AmcrestCamera channel, since this then requires only a single connection to the NVR which can then use all channels, rather than needing to have separate connections per channel, which is why I implemented it that way.

[akram]

closing this in favor of #174 kudos to @flacjacket