Return custom objects that implements Sequence protocol, from fetch_capture_waveform()#1430
Return custom objects that implements Sequence protocol, from fetch_capture_waveform()#1430
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1430 +/- ##
=========================================
Coverage 91.90% 91.90%
=========================================
Files 20 32 +12
Lines 3642 7008 +3366
=========================================
+ Hits 3347 6441 +3094
- Misses 295 567 +272
Continue to review full report at Codecov.
|
| self._capture_waveform_data = capture_waveform_data | ||
|
|
||
| def __repr__(self): | ||
| return str(self) |
There was a problem hiding this comment.
From https://docs.python.org/3.4/reference/datamodel.html#object.__repr__
If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment).
Usually this means You'll want the __module__ and __qualname__ of the class in there along with the args.
E.g.
def __repr__(self):
return "{}.{}({})".format(self.__class__.__module__, self.__class__.__qualname__, ...)There was a problem hiding this comment.
Usually this means You'll want the module and qualname of the class in there along with the args.
This is good to know. I couldn't find any best practice tips for __repr__ online. I checked a couple of popular external libraries and didn't find them including __module__:
>>> import numpy as np
>>> n = np.arange(2)
>>> repr(n)
'array([0, 1])'
>>> import pandas as pd
>>> t = pd.CategoricalDtype(categories=['b', 'a'], ordered=True)
>>> repr(t)
"CategoricalDtype(categories=['b', 'a'], ordered=True)"
There was a problem hiding this comment.
Look at internal libraries, as they're probably the best citizens:
>>>import datetime
>>> datetime.datetime(1, 2, 3)
datetime.datetime(1, 2, 3, 0, 0)
>>> import operator
>>> operator.attrgetter("foo")
operator.attrgetter('foo')Although that's not consistent either:
>>> import collections
>>> collections.OrderedDict()
OrderedDict()🤷♂️ Since you're defining this same type in several libraries, it might be good to tell the user which library it came from.
| class CaptureWaveform(collections.abc.Sequence): | ||
| '''Used by fetch_capture_waveform() to return the capture waveform data''' | ||
| def __init__(self, capture_waveform_data): | ||
| self._capture_waveform_data = capture_waveform_data |
There was a problem hiding this comment.
The class is already named "CaptureWaveform", so having the argument and attribute also have "capture waveform" in it is superfluous.
|
PR rejected. See here for details. |
2 participants
This contribution adheres to CONTRIBUTING.md.
I've updated CHANGELOG.md if applicable.
I've added tests applicable for this pull request
What does this Pull Request accomplish?
CaptureWaveformto implementscollections.abc.Sequenceinterfacefetch_capture_waveform()to returnCaptureWaveformobjects in the dictionaryNote: The PR is not complete. I'm creating a draft version to get some initial feedback before spending more time on it. The following items have not been done yet:
List issues fixed by this Pull Request below, if any.
What testing has been done?
New tests added. CI will run all tests.