EXPORT_DATA2 support #17
Conversation
- Changed _build(self, s), now also supports lists as its argument - Fixed bug in set_active_channels where it would send malformed channel lists when empty - Added new export_data
| digital_no = 0 if digital is None else len(digital) | ||
| analog_no = 0 if analog is None else len(analog) | ||
| if digital_no <= 0 and analog_no <= 0: | ||
| raise ValueError('bad active channels, has to set at least one active channel') |
There was a problem hiding this comment.
Elsewhere, I raise a ImpossibleSettings (derived from Saleae.SaleaeError) for things like this. I think that's slightly more helpful than a generic ValueError (although it may may sense to mix-in ValueError to the ImpossibleSettings class).
There was a problem hiding this comment.
I agree, previously the code was mainly using NotImplementedError errors in that area and I did not see the ImpossibleSettings exception.
Just for uniformity, why is ImpossibleSettings plural and not singular? Any particular reasons?
|
Thanks for the PR! Lot of nice stuff here. I'd rather not remove the old EXPORT_DATA command. Yes, it's deprecated, but I'm sure there are people using it. How about renaming the old method |
|
Thanks! That is a hard one, Saleae noted that they deprecated EXPORT_DATA and it was broken for me anyways. I've tried to keep the arguments the same for old export_data but seeing as I'm using the kwargs forwarding you need to drop the "_" prefixes for the formatter arguments. I could add some code to properly translate the old arguments to the new arguments to not break old code. Or just bump major version and force people to update if they want to use EXPORT_DATA2 else they are stuck on old version (lets hope they are using requirements.txt correctly). To be honest, I do not like to support broken/deprecated code, it is a great way to add cruft to a otherwise well maintained code base. That said, you are the owner so you have the final call about that. Let me know what you would like and I will tailor the PR to whatever your choice is on that matter. |
|
Ping, forgot about thins merge request. Updated my branch to head, and processed your feedback. Please consider merging so we can fix this outstanding MR. Thanks |
|
Hey, sorry, I'd forgotten about it too, then the other one last Friday made me notice it again, then I remembered it was big, then I went down the rabbit hole of getting CI set up first, then I exhausted my time... sorry. I'll try to look back at this in the next day or two. A quick glance at that CI failure looks like it might have exposed a bug in the Logic software ;) |
|
While I agree that leaving legacy code lying around isn't the best, my reading of this part of Saleae's documentation is that they aren't planning on removing the old export_data command, so I don't feel like this library should either:
I re-jiggered your commits just a bit so that they squashed the changes and renamed to an export_data2 function to match the saleae api. Have a quick look here: https://github.com/ppannuto/python-saleae/tree/export_data2 If that looks good to you, I'll merge this in. |
|
Cool, looks good to me! Thanks for your help :) |
According to #7, the EXPORT_DATA is deprecated. I also was unable to get EXPORT_DATA to work in my use case. I've added support for the EXPORT_DATA2 command and all exports are working.